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

@@ -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']

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}),

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

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