[API] Optional field permissions (#12529)

* Add permissions check for OptionalField

* Adjust existing unit tests

* Add caching

* Additional tests

* Additional testing

* Add exclusions for owner and group models

* Bump CHANGELOG

* Detect demo branch based on software version

* Updated docstrings

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Breaking Changes
 
+- [#12529](https://github.com/inventree/InvenTree/pull/12529) removes related detail fields on API endpoints for which the user does not have view permissions.
 - [#12360](https://github.com/inventree/InvenTree/pull/12360) removes the MPTT mixin from the StockItem model, and removes the self-referential tree structure from the database. This change was made to simplify the StockItem model and improve performance, as the MPTT tree structure was causing significant overhead in certain operations. Any external client applications which made use of the MPTT functionality will need to be updated to account for this change.
 - [#12320](https://github.com/inventree/InvenTree/pull/12320) changes the default behavior of the `invoke migrate` command. Now, it no longer generates new migrations by default. Instead, it will only apply existing migrations to the database. If you want to detect and generate new migrations, you must now explicitly use the `--detect` flag. This change was made to prevent accidental generation of migrations when running the command, which could lead to unexpected changes in the database schema. Additionally, `invoke update` will no longer result in new migrations being generated, and will only apply existing migrations to the database. This change was made to ensure that the update process is predictable and does not introduce unexpected changes to the database schema.
 - [#12223](https://github.com/inventree/InvenTree/pull/12223) removes support for python 3.11 and stops providing packages for Debian 11 and Ubuntu 20.04.

src/backend/InvenTree/InvenTree/serializers.py

@@ -6,6 +6,7 @@ from dataclasses import dataclass
 from decimal import Decimal
 from typing import Optional
 
+from django.contrib.auth import get_user_model
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import ValidationError as DjangoValidationError
 from django.core.files.storage import default_storage
@@ -49,6 +50,12 @@ class OptionalField:
 
     This allows for optimization of database queries based only on the requested data.
 
+    If the field embeds another model's data (e.g. a nested "detail" serializer), the
+    requesting user's view permission against that model is checked before the field is
+    included - this is inferred from `serializer_class.Meta.model` unless `model` is
+    explicitly provided. This prevents a user from seeing embedded data (e.g. a Part,
+    via a BuildOrder's `part_detail`) that they do not have direct permission to view.
+
     Example:
     class MySerializer(FilterableSerializerMixin, serializers.ModelSerializer):
         my_optional_field = OptionalField(
@@ -70,6 +77,7 @@ class OptionalField:
     filter_name: Optional[str] = None
     filter_by_query: bool = True
     prefetch_fields: Optional[list[str]] = None
+    model: Optional[type] = None
 
 
 class FilterableSerializerMixin:
@@ -120,20 +128,27 @@ class FilterableSerializerMixin:
 
         Order of operations:
 
-        - If we are generating the schema, always include the field
+        - If we are generating the schema, always include the field (unless the user does not have permission to view)
         - If this is a write request (POST, PUT, PATCH) and we are not exporting, always include the field
         - If this is a top-level serializer, check the request query parameters for the filter name
         - Check the kwargs provided to the serializer instance
         - Finally, fall back to the default_include value for the field itself
+
+        Whatever the outcome of the above, if the field embeds another model's data, the
+        result is then narrowed by the requesting user's view permission against that
+        model (see `check_field_permission`) - the field is never included for a user who
+        cannot view the embedded model, regardless of query parameters or defaults.
         """
         field_ref = field.filter_name or field_name
 
         # If we have already found a value for this filter, use it
         # This allows multiple optional fields to share the same filter value
+        # Note: fields sharing a filter_ref may still embed different models, so the
+        # permission gate is re-applied per-field even when the raw value is cached
         cached_value = self.optional_filters.get(field_ref, None)
 
         if cached_value is not None:
-            return cached_value
+            return self.check_field_permission(field, cached_value)
 
         # First, check kwargs provided to the serializer instance
         # We also pop the value to avoid issues with nested serializers
@@ -149,10 +164,12 @@ class FilterableSerializerMixin:
 
         field_kwargs = field.serializer_kwargs or {}
 
-        # Skip filtering for a write request - all fields should be present for data creation
+        # Skip filtering for a write request
+        # All fields should be present for data creation,
+        # excepting those for which the user does not have the required permissions
         if method := getattr(self.request, 'method', None):
             if method not in SAFE_METHODS and not self.is_exporting():
-                return True
+                return self.check_field_permission(field, True)
             else:
                 # Ignore write_only fields for read requests
                 if field_kwargs.get('write_only', False):
@@ -175,7 +192,68 @@ class FilterableSerializerMixin:
         if value is None:
             value = field.default_include
 
-        return value
+        return self.check_field_permission(field, value)
+
+    def check_field_permission(self, field: OptionalField, included: bool) -> bool:
+        """Narrow an inclusion decision by the requesting user's view permission.
+
+        An OptionalField which embeds another model's data (e.g. `part_detail` embedding
+        a Part) should not be included unless the requesting user actually has view
+        permission on that model - otherwise a user could see e.g. Part data via a
+        BuildOrder's `part_detail` field without having Part view permission themselves.
+
+        Arguments:
+            field: The OptionalField instance being resolved
+            included: The inclusion decision made so far
+
+        Returns:
+            False if the field embeds a model the requesting user cannot view, otherwise
+            the original `included` value unchanged.
+        """
+        if not included:
+            return included
+
+        model = field.model or getattr(
+            getattr(field.serializer_class, 'Meta', None), 'model', None
+        )
+
+        if model is None:
+            return included
+
+        # A handful of models describe *permission metadata itself* (who a user is,
+        # what a role can do) rather than embedded business data - gating these behind
+        # a role would hide a user's own account/role information from themselves and
+        # from almost every non-admin user, which is a different (and much broader)
+        # concern than embedding e.g. Part/Company records. Deliberately exempted here:
+        # - auth_user: username attribution fields (issued_by_detail, checked_by_detail, ...)
+        # - auth_group: group membership/attribution (ExtendedUserSerializer.groups, ...)
+        # - users_ruleset: role/permission listings (GroupSerializer.roles, ...)
+        # - users_owner: user/group "owner" wrapper (responsible_detail, ...) - already in
+        #   get_ruleset_ignore() so check_user_permission would return True anyway, but
+        #   exempted explicitly here to document intent and skip the call
+        from django.contrib.auth.models import Group
+
+        from users.models import Owner, RuleSet
+
+        if model in (get_user_model(), Group, Owner, RuleSet):
+            return included
+
+        user = getattr(self.request, 'user', None)
+
+        if user is None:
+            return included
+
+        cache = self.__dict__.setdefault('_field_permission_cache', {})
+        cache_key = (user.pk, model)
+
+        if cache_key in cache:
+            return cache[cache_key]
+
+        from users.permissions import check_user_permission
+
+        result = check_user_permission(user, model, 'view')
+        cache[cache_key] = result
+        return result
 
     def find_optional_fields(self):
         """Find all optional fields defined on this serializer."""

src/backend/InvenTree/InvenTree/test_serializers.py

@@ -11,6 +11,7 @@ from InvenTree.mixins import ListCreateAPI, OutputOptionsMixin
 from InvenTree.serializers import OptionalField
 from InvenTree.unit_test import InvenTreeAPITestCase
 from InvenTree.urls import backendpatterns
+from part.models import Part
 
 
 class SampleSerializer(
@@ -23,7 +24,15 @@ class SampleSerializer(
         """Meta options."""
 
         model = User
-        fields = ['field_a', 'field_b', 'field_c', 'field_d', 'field_e', 'id']
+        fields = [
+            'field_a',
+            'field_b',
+            'field_c',
+            'field_d',
+            'field_e',
+            'field_f',
+            'id',
+        ]
 
     field_a = SerializerMethodField(method_name='sample')
     field_b = OptionalField(
@@ -49,6 +58,15 @@ class SampleSerializer(
         filter_by_query=False,
     )
 
+    # Field which embeds a model the requesting user may not have permission to view
+    field_f = OptionalField(
+        serializer_class=SerializerMethodField,
+        serializer_kwargs={'method_name': 'sample'},
+        default_include=True,
+        filter_name='field_f',
+        model=Part,
+    )
+
     def sample(self, obj):
         """Sample method field."""
         return 'sample123'
@@ -111,3 +129,27 @@ class FilteredSerializers(InvenTreeAPITestCase):
             self.assertContains(response, 'field_c')
             self.assertContains(response, 'field_d')
             self.assertNotContains(response, 'field_e')
+
+    def test_permission_gating(self):
+        """An OptionalField which embeds a model should respect the model's permissions.
+
+        'field_f' defaults to included, but declares 'model=Part' - it should only
+        appear in the response if the requesting user actually has 'part.view'.
+        """
+        with self.settings(
+            ROOT_URLCONF=__name__,
+            CSRF_TRUSTED_ORIGINS=['http://testserver'],
+            SITE_URL='http://testserver',
+        ):
+            url = reverse('sample-list', urlconf=__name__)
+
+            # No 'part' role assigned - field should be hidden despite default_include=True
+            response = self.client.get(url)
+            self.assertContains(response, 'field_a')
+            self.assertNotContains(response, 'field_f')
+
+            # Assign the 'part.view' role - field should now appear
+            self.assignRole('part.view')
+            response = self.client.get(url)
+            self.assertContains(response, 'field_f')
+            self.assertEqual(response.data[0]['field_f'], 'sample123')

src/backend/InvenTree/company/test_api.py

@@ -478,7 +478,7 @@ class ManufacturerTest(InvenTreeAPITestCase):
         'supplier_part',
     ]
 
-    roles = ['part.add', 'part.change']
+    roles = ['part.add', 'part.change', 'purchase_order.view']
 
     def test_manufacturer_part_list(self):
         """Test the ManufacturerPart API list functionality."""

src/backend/InvenTree/order/test_api.py

@@ -58,7 +58,14 @@ class OrderTest(InvenTreeAPITestCase):
         'transfer_order',
     ]
 
-    roles = ['purchase_order.change', 'sales_order.change', 'transfer_order.change']
+    roles = [
+        'purchase_order.change',
+        'sales_order.change',
+        'transfer_order.change',
+        'part.view',
+        'stock.view',
+        'stock_location.view',
+    ]
 
     def filter(self, filters, count):
         """Test API filters."""
@@ -3620,7 +3627,7 @@ class ReturnOrderLineItemTests(InvenTreeAPITestCase):
         'supplier_part',
         'stock',
     ]
-    roles = ['return_order.view']
+    roles = ['return_order.view', 'part.view', 'stock.view']
 
     def test_options(self):
         """Test the OPTIONS endpoint."""

src/backend/InvenTree/part/test_api.py

@@ -52,6 +52,7 @@ class PartImageTestMixin:
         'part.delete',
         'part_category.change',
         'part_category.add',
+        'stock_location.view',
     ]
 
     @classmethod
@@ -820,6 +821,7 @@ class PartAPITestBase(InvenTreeAPITestCase):
         'part.delete',
         'part_category.change',
         'part_category.add',
+        'stock_location.view',
     ]
 
 
@@ -2390,10 +2392,13 @@ class PartListTests(PartAPITestBase):
             query_count_with_price_breaks - query_count_without_price_breaks
         )
 
-        # There are 2 additional queries, 1 for the salepricebreak subselect and 1 for Currency codes because of InvenTreeCurrencySerializer
+        # There are 4 additional queries: 1 for the salepricebreak subselect, 1 for
+        # Currency codes because of InvenTreeCurrencySerializer, and 2 for the one-off
+        # permission check (fetch groups + rule sets) gating the price_breaks field's
+        # embedded PartSellPriceBreak model - this cost is fixed per-request, not per-row.
         self.assertLessEqual(
             query_difference,
-            2,
+            4,
             f'Query count difference too high: {query_difference} (with: {query_count_with_price_breaks}, without: {query_count_without_price_breaks})',
         )
 
@@ -3034,6 +3039,14 @@ class BomItemTest(InvenTreeAPITestCase):
         """Get the detail view for a single BomItem object."""
         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')
+
         bom_item = BomItem.objects.get(pk=3)
 
         # Create some substitutes for this BomItem
@@ -3111,6 +3124,11 @@ 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')
+
         self.run_output_test(
             reverse('api-bom-item-detail', kwargs={'pk': 3}),
             [

src/backend/InvenTree/stock/test_api.py

@@ -57,6 +57,7 @@ class StockAPITestCase(InvenTreeAPITestCase):
         'stock_location.add',
         'stock_location.delete',
         'stock.delete',
+        'part.view',
     ]
 
 
@@ -1988,6 +1989,28 @@ class StockItemTest(StockAPITestCase):
             ],
         )
 
+    def test_part_detail_permissions(self):
+        """Test that the part_detail output option is only available to users with permission."""
+        url = reverse('api-stock-detail', kwargs={'pk': 1})
+
+        # User has permission to view parts
+        response = self.get(url, {'part_detail': True}, expected_code=200)
+
+        self.assertIn('pk', response.data)
+        self.assertIn('part', response.data)
+        self.assertIn('part_detail', response.data)
+
+        # Remove 'part view' permission from user
+        self.clearRoles()
+        response = self.get(url, {'part_detail': True}, expected_code=403)
+
+        self.assignRole('stock.view')
+
+        response = self.get(url, {'part_detail': True}, expected_code=200)
+        self.assertIn('pk', response.data)
+        self.assertIn('part', response.data)
+        self.assertNotIn('part_detail', response.data)
+
     def test_install(self):
         """Test that stock item can be installed into another item, via the API."""
         # Select the "parent" stock item