From ddd460391e258c53bda9a31967aa990cab8d1391 Mon Sep 17 00:00:00 2001 From: Oliver Date: Sat, 17 Oct 2026 14:44:46 +1100 Subject: [PATCH] Permission bug fixes: (#13033) Bug 1: Inherited BOM permissions flip on every save Bug 2: Unused `permission_required` in PartInternalPriceList --- src/backend/InvenTree/part/api.py | 1 - src/backend/InvenTree/part/test_api.py | 14 +++----- src/backend/InvenTree/users/tasks.py | 36 +++++++-------------- src/backend/InvenTree/users/tests.py | 45 ++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 36 deletions(-) diff --git a/src/backend/InvenTree/part/api.py b/src/backend/InvenTree/part/api.py index 6cebb65686..ea4559e4e6 100644 --- a/src/backend/InvenTree/part/api.py +++ b/src/backend/InvenTree/part/api.py @@ -399,7 +399,6 @@ class PartInternalPriceList(DataExportViewMixin, ListCreateAPI): queryset = PartInternalPriceBreak.objects.all() serializer_class = part_serializers.PartInternalPriceSerializer - permission_required = 'roles.sales_order.show' filter_backends = SEARCH_ORDER_FILTER filterset_fields = ['part'] diff --git a/src/backend/InvenTree/part/test_api.py b/src/backend/InvenTree/part/test_api.py index 8b12a8bfd2..1138c629dd 100644 --- a/src/backend/InvenTree/part/test_api.py +++ b/src/backend/InvenTree/part/test_api.py @@ -3074,12 +3074,8 @@ class BomItemTest(InvenTreeAPITestCase): from part.models import BomItemSubstitute # Viewing 'substitutes' requires the 'bom' role (BomItemSubstitute is not - # covered by the part->bomitem RULESET_CHANGE_INHERIT fallback). Grant both - # 'add' and 'delete' so the 'bom' RuleSet ends up fully matching the existing - # part-inherited bomitem permissions - granting only 'view' would otherwise - # cause update_group_roles() to wipe those already-inherited permissions. - self.assignRole('bom.add') - self.assignRole('bom.delete') + # covered by the part->bomitem RULESET_CHANGE_INHERIT fallback) + self.assignRole('bom.view') bom_item = BomItem.objects.get(pk=3) @@ -3158,10 +3154,8 @@ class BomItemTest(InvenTreeAPITestCase): def test_output_options(self): """Test that various output options work as expected.""" - # Viewing 'substitutes' requires the 'bom' role (see test_get_bom_detail for why - # both 'add' and 'delete' are granted together). - self.assignRole('bom.add') - self.assignRole('bom.delete') + # Viewing 'substitutes' requires the 'bom' role + self.assignRole('bom.view') self.run_output_test( reverse('api-bom-item-detail', kwargs={'pk': 3}), diff --git a/src/backend/InvenTree/users/tasks.py b/src/backend/InvenTree/users/tasks.py index 77a903a862..491c233e2b 100644 --- a/src/backend/InvenTree/users/tasks.py +++ b/src/backend/InvenTree/users/tasks.py @@ -88,7 +88,7 @@ def update_group_roles(group: Group, debug: bool = False) -> None: if action not in ['view', 'add', 'change', 'delete']: # pragma: no cover raise ValueError(f'Action {action} is invalid') - permission_string = get_model_permission_string(model, action) + permission_string = get_model_permission_string(name, action) if allowed: # An 'allowed' action is always preferenced over a 'forbidden' action @@ -125,6 +125,16 @@ def update_group_roles(group: Group, debug: bool = False) -> None: add_model(model, 'change', ruleset.can_change) add_model(model, 'delete', ruleset.can_delete) + # Enable action permissions for certain children models, + # if the parent model has the same action permission. + # Note: This must be determined from the *desired* permissions (rather than + # the permissions currently assigned to the group), and before any permissions are + # added or removed - otherwise inherited permissions are dropped on alternate updates. + for parent, child in RULESET_CHANGE_INHERIT: + for action in ['view', 'change', 'add', 'delete']: + if f'{parent}.{action}_{parent}' in permissions_to_add: + add_model(f'{parent}_{child}', action, True) + def get_permission_object(permission_string): """Find the permission object in the database, from the simplified permission string. @@ -171,27 +181,3 @@ def update_group_roles(group: Group, debug: bool = False) -> None: group.permissions.remove(permission) if debug: # pragma: no cover logger.debug('Removing permission %s from group %s', perm, group.name) - - # Enable all action permissions for certain children models - # if parent model has 'change' permission - for parent, child in RULESET_CHANGE_INHERIT: - parent_child_string = f'{parent}_{child}' - - # Check each type of permission - for action in ['view', 'change', 'add', 'delete']: - parent_perm = f'{parent}.{action}_{parent}' - - if parent_perm in group_permissions: - child_perm = f'{parent}.{action}_{child}' - - # Check if child permission not already in group - if child_perm not in group_permissions: - # Create permission object - add_model(parent_child_string, action, ruleset.can_delete) - # Add to group - permission = get_permission_object(child_perm) - if permission: - group.permissions.add(permission) - logger.debug( - 'Adding permission %s to group %s', child_perm, group.name - ) diff --git a/src/backend/InvenTree/users/tests.py b/src/backend/InvenTree/users/tests.py index be206f3994..f21efa2be8 100644 --- a/src/backend/InvenTree/users/tests.py +++ b/src/backend/InvenTree/users/tests.py @@ -194,6 +194,51 @@ class RuleSetModelTest(TestCase): # There should now not be any permissions assigned to this group self.assertEqual(group.permissions.count(), 0) + def test_permission_inherit_stable(self): + """Inherited child permissions must survive repeated group updates. + + The 'part' role grants permissions against 'part_bomitem' via RULESET_CHANGE_INHERIT, + even though 'part_bomitem' is not covered by the 'part' role directly. + These inherited permissions must not be dropped (or toggled) on subsequent updates. + """ + group = Group.objects.create(name='Inherit group') + + part_rule = group.rule_sets.get(name='part') + part_rule.can_add = True + part_rule.can_delete = True + part_rule.save() + + expected = { + f'part.{action}_bomitem' for action in ['view', 'add', 'change', 'delete'] + } + + for _ in range(4): + group.save() + + permissions = { + f'{p.content_type.app_label}.{p.codename}' + for p in group.permissions.all().prefetch_related('content_type') + } + + self.assertTrue(expected.issubset(permissions)) + + # Removing the parent permissions also removes the inherited permissions + part_rule.can_view = False + part_rule.can_add = False + part_rule.can_change = False + part_rule.can_delete = False + part_rule.save() + + for _ in range(2): + group.save() + + permissions = { + f'{p.content_type.app_label}.{p.codename}' + for p in group.permissions.all().prefetch_related('content_type') + } + + self.assertTrue(expected.isdisjoint(permissions)) + class OwnerModelTest(InvenTreeTestCase): """Some simplistic tests to ensure the Owner model is setup correctly."""