From 57d04db7e1cd98bf5bea4b045b77484151325091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Warnon?= Date: Tue, 24 May 2022 14:23:43 +0000 Subject: [PATCH] [FW][FIX] knowledge: correctly propagate to children when moving to private Currently, when moving an article to the private section, its children are untouched and keep their access rights. This is incorrect: when making an article private, all (accessible) children should be made private as well. See 'knowledge.article#_make_private' for details. An exception is made for children articles to which the user does not have a write access to. Those are handled as the following (same as archiving flow): - They become independent from the parent - They are moved as "root articles" - The potential specific member access from their parent(s) are copied To avoid loosing access if specific members where configured on the parent(s) of those articles, since they will no-longer inherit from them. Tests were added to enforce this behavior. Task-2859616 X-original-commit: odoo/odoo@82af404c5b281efae090c7c181e460b98e2575f1 Part-of: odoo/odoo#95471 --- addons/knowledge/models/knowledge_article.py | 89 ++++++---- .../tests/test_knowledge_article_business.py | 159 ++++++++++++++++++ 2 files changed, 216 insertions(+), 32 deletions(-) diff --git a/addons/knowledge/models/knowledge_article.py b/addons/knowledge/models/knowledge_article.py index 1e9bf3b1287..1ff07ba61f1 100644 --- a/addons/knowledge/models/knowledge_article.py +++ b/addons/knowledge/models/knowledge_article.py @@ -8,7 +8,7 @@ from markupsafe import Markup from werkzeug.urls import url_join from odoo import Command, fields, models, api, _ -from odoo.exceptions import AccessError, ValidationError +from odoo.exceptions import AccessError, ValidationError, UserError from odoo.osv import expression from odoo.tools import get_lang @@ -1190,6 +1190,10 @@ class Article(models.Model): with write access. Requires a sudo to bypass member ACLs after checking write access on the article. + Children articles to which the user also has a write access to are made + private as well. Other articles are detached, see '_detach_unwritable_descendants' + for details. + :param parent: an optional parent to move the article under; :param before_article: article before which the article should be moved. Otherwise it is put as last parent children; @@ -1200,47 +1204,68 @@ class Article(models.Model): parent = parent if parent is not False else self.env['knowledge.article'] before_article = before_article if before_article is not False else self.env['knowledge.article'] - article_values = {'parent_id': parent.id} + try: + self.check_access_rights('write') + (self + parent).check_access_rule('write') + except (AccessError, UserError): + if parent: + raise AccessError( + _("You are not allowed to move '%(article_name)s' under '%(parent_name)s'.", + article_name=self.display_name, + parent_name=parent.display_name) + ) + raise AccessError( + _("You are not allowed to make '%(article_name)s' private.", article_name=self.display_name) + ) + + # first detach unwritable children (see ``_detach_unwritable_descendants``) + writable_descendants = self._detach_unwritable_descendants() + + article_values = { + # reset internal permission if parent is set (will inherit) or force private (aka 'none') + 'internal_permission': False if parent else 'none', + # cannot be desync when made private as we wipe members access + 'is_desynchronized': False, + 'parent_id': parent.id if parent else False, + } if before_article: article_values['sequence'] = before_article.sequence - # when moving as root: imply member manipulations hence sudo and forced ACL check - writable_self = self + self_sudo = self.sudo() + # remove members as the article is moved to private + members_to_remove = self_sudo.article_member_ids + self_member_command = [] if not parent: - try: - self.check_access_rights('write') - (self + parent).check_access_rule('write') - except: - raise AccessError( - _("You are not allowed to move this article under article %(parent_name)s", - parent_name=parent.display_name) - ) - - # be sure to have an internal permission on the article if moved outside - # of an hierarchy - article_values.update({ - 'internal_permission': 'none', - 'is_desynchronized': False, - }) - - writable_self = self.sudo() - self_member = writable_self.article_member_ids.filtered(lambda m: m.partner_id == self.env.user.partner_id) - - # first add or update the new membership with 'write' access + # make sure the current user is the only member left with write access to the article + # if we have a parent, this is not necessary as we will inherit members access from them + self_member = self_sudo.article_member_ids.filtered(lambda m: m.partner_id == self.env.user.partner_id) if self_member: - write_member_command = [(1, self_member.id, {'permission': 'write'})] + self_member_command = [(1, self_member.id, {'permission': 'write'})] + # keep current member + members_to_remove = members_to_remove.filtered( + lambda member: member.partner_id != self.env.user.partner_id + ) else: - write_member_command = [(0, 0, { + self_member_command = [(0, 0, { 'partner_id': self.env.user.partner_id.id, 'permission': 'write' })] - # then remove other members as the article is moved to private - article_values['article_member_ids'] = [ - (2, member.id) - for member in writable_self.article_member_ids if member.partner_id != self.env.user.partner_id - ] + write_member_command - return writable_self.with_context(knowledge_member_skip_writable_check=True).write(article_values) + article_values['article_member_ids'] = [ + (2, member.id) + for member in members_to_remove + ] + self_member_command + + res = self_sudo.with_context(knowledge_member_skip_writable_check=True).write(article_values) + + # remove all specific memberships configurations on children. They now inherit + # the only 'write' member from their parent now, making them private. + writable_descendants.with_context(knowledge_member_skip_writable_check=True).write({ + 'internal_permission': False, + 'article_member_ids': [(5, 0)], + }) + + return res def _has_write_member(self, partners_to_exclude=False, members_to_exclude=False): """ Method allowing to check if this article still has at least one member diff --git a/addons/knowledge/tests/test_knowledge_article_business.py b/addons/knowledge/tests/test_knowledge_article_business.py index aebe0c8abcb..cb1b7ddcc10 100644 --- a/addons/knowledge/tests/test_knowledge_article_business.py +++ b/addons/knowledge/tests/test_knowledge_article_business.py @@ -476,6 +476,165 @@ class TestKnowledgeArticleBusiness(KnowledgeCommonWData): playground_articles_asmanager = playground_articles.with_user(self.user_employee_manager) self.assertEqual(playground_articles_asmanager.mapped('is_user_favorite'), [False, False, False]) + @mute_logger('odoo.addons.base.models.ir_rule', 'odoo.models.unlink') + @users('employee') + def test_article_make_private(self): + """ Testing the API that makes an article 'private'. Making an article + private generally: + - sets internal_permission 'none'; + - sets current environment user as only write member; + + A lot of extra post-processing is applied, see ``KnowledgeArticle. + _move_and_make_private()`` for details. + + Specific setup for this test + # - Playground workspace w+ (customer-r+) + # - Child1 " " (customer-r+) + # - Gd Child1 " " + # - Gd Child2 " " + # - GdGd Child1 " " + # - Child2 " " + # - Gd Child1 " " + # - GdGd Child " " + # - ReadMember Child " w+ (employee-r+) + # - ReadInternal Child " r+ (employee2-w+) + # - Hidden Child " w+ (employee-no) + """ + article_workspace = self.article_workspace.with_env(self.env) + workspace_children = self.workspace_children.with_env(self.env) + wkspace_grandchildren = self.wkspace_grandchildren.with_env(self.env) + wkspace_grandgrandchildren = self.wkspace_grandgrandchildren.with_env(self.env) + + # add an additional member on 'article_workspace' and one of its children for further checks + (self.article_workspace + self.workspace_children[0]).write({ + 'article_member_ids': [(0, 0, { + 'partner_id': self.customer.id, + 'permission': 'read', + })] + }) + + # add 3 extra articles for further checks + # - one to which 'employee' only has 'read' access (as member) + # - one to which 'employee' only has 'read' access (internal) + # - one invisible, "employee2" has access to that one in write mode + [wkspace_child_read_member_access, + wkspace_child_read_internal_access, + wkspace_child_no_access] = self.env['knowledge.article'].sudo().create([ + { + 'article_member_ids': [(0, 0, { + 'partner_id': self.partner_employee.id, + 'permission': 'read', + })], + 'internal_permission': 'write', + 'name': 'Read Member Child', + 'parent_id': article_workspace.id, + }, { + 'article_member_ids': [(0, 0, { + 'partner_id': self.partner_employee2.id, + 'permission': 'write', + })], + 'internal_permission': 'read', + 'name': 'Read Internal Child', + 'parent_id': article_workspace.id, + }, { + 'article_member_ids': [(0, 0, { + 'partner_id': self.partner_employee.id, + 'permission': 'none', + })], + 'internal_permission': 'write', + 'name': 'Hidden Child', + 'parent_id': article_workspace.id, + } + ]) + with self.assertRaises(exceptions.AccessError): + wkspace_child_no_access.with_env(self.env).body + article_workspace._move_and_make_private() + + # 1. main article was correctly moved to private + self.assertEqual(article_workspace.category, 'private') + self.assertEqual(article_workspace.internal_permission, 'none') + self.assertMembers( + article_workspace, + 'none', + {self.partner_employee: 'write'} + ) + + # 2. accessible children were correctly moved to private + for workspace_descendant, parent_id in zip( + workspace_children + wkspace_grandchildren + wkspace_grandgrandchildren, + [article_workspace, article_workspace, workspace_children[0], workspace_children[0], + workspace_children[1], wkspace_grandchildren[1], wkspace_grandchildren[2]] + ): + self.assertEqual(workspace_descendant.category, 'private') + self.assertEqual(workspace_descendant.inherited_permission_parent_id, article_workspace) + self.assertEqual(workspace_descendant.parent_id, parent_id) + # all specific members should have been wiped, no permission + self.assertMembers( + workspace_descendant, + False, + {} + ) + + # 3.children that were not writable are moved as a root articles and that + # members / internal permissions are kept + self.assertEqual(wkspace_child_read_member_access.category, 'workspace') + self.assertFalse(wkspace_child_read_member_access.parent_id) + self.assertMembers( + wkspace_child_read_member_access, + 'write', + {self.partner_employee: 'read', + self.customer: 'read'} + ) + self.assertEqual(wkspace_child_read_internal_access.category, 'workspace') + self.assertFalse(wkspace_child_read_internal_access.parent_id) + self.assertMembers( + wkspace_child_read_internal_access, + 'read', + {self.partner_employee2: 'write', + self.customer: 'read'} + ) + self.assertEqual(wkspace_child_no_access.category, 'workspace') + self.assertFalse(wkspace_child_no_access.parent_id) + self.assertMembers( + wkspace_child_no_access, + 'write', + {self.partner_employee: 'none', + self.customer: 'read'} + ) + + # 'Hidden Child' is still not accessible for employee + with self.assertRaises(exceptions.AccessError): + wkspace_child_no_access.with_env(self.env).body + wkspace_child_no_access.with_user(self.user_employee2).body + + @mute_logger('odoo.addons.base.models.ir_rule', 'odoo.models.unlink') + @users('employee_manager') + def test_article_make_private_w_parent(self): + """ Test a special case when making private: moving under an existing private parent. """ + article_shared = self.article_shared.with_env(self.env) + article_private_manager = self.article_private_manager.with_env(self.env) + + # first test that making 'article_shared' fails since 'employee_manager' + # does not have write access to it (only read) + with self.assertRaises(exceptions.AccessError): + article_shared._move_and_make_private(parent=article_private_manager) + + # then grant write access to test the flow + article_shared.sudo().article_member_ids.filtered( + lambda member: member.partner_id == self.partner_employee_manager + ).write({'permission': 'write'}) + + article_shared._move_and_make_private(parent=article_private_manager) + + self.assertEqual(article_shared.category, 'private') + # the internal permission should not be set as we inherit from our private parent + # members should be wiped as we inherit from our private parent + self.assertMembers( + article_shared, + False, + {} + ) + @mute_logger('odoo.addons.base.models.ir_rule') @users('employee') def test_article_move_to(self):