Dynamic group permissions improvements (nautobot#5757)

* Enforce user object permissions when viewing Dynamic Group members

* Also enforce user permissions when viewing the DGs that an object is a member of

changes/5757.security

@@ -0,0 +1 @@
+Fixed missing member object permission enforcement (e.g., enforce Device permissions for a Dynamic Group containing Devices) when viewing Dynamic Group member objects in the UI or REST API ([GHSA-qmjf-wc2h-6x3q](https://github.com/nautobot/nautobot/security/advisories/GHSA-qmjf-wc2h-6x3q)).

nautobot/core/testing/views.py

@@ -213,6 +213,8 @@ def test_get_object_with_permission(self):
                             escape(str(instance.cf.get(custom_field.key) or "")), response_body, msg=response_body
                         )
 
+            return response  # for consumption by child test cases if desired
+
         @override_settings(EXEMPT_VIEW_PERMISSIONS=[])
         def test_get_object_with_constrained_permission(self):
             instance1, instance2 = self._get_queryset().all()[:2]
@@ -230,11 +232,14 @@ def test_get_object_with_constrained_permission(self):
             obj_perm.object_types.add(ContentType.objects.get_for_model(self.model))
 
             # Try GET to permitted object
-            self.assertHttpStatus(self.client.get(instance1.get_absolute_url()), 200)
+            response = self.client.get(instance1.get_absolute_url())
+            self.assertHttpStatus(response, 200)
 
             # Try GET to non-permitted object
             self.assertHttpStatus(self.client.get(instance2.get_absolute_url()), 404)
 
+            return response  # for consumption by child test cases if desired
+
         @override_settings(EXEMPT_VIEW_PERMISSIONS=[])
         def test_has_advanced_tab(self):
             instance = self._get_queryset().first()

nautobot/extras/api/views.py

@@ -304,13 +304,13 @@ class DynamicGroupViewSet(NotesViewSetMixin, ModelViewSet):
     # @extend_schema(methods=["get"], responses={200: member_response})
     @action(detail=True, methods=["get"])
     def members(self, request, pk, *args, **kwargs):
-        """List member objects of the same type as the `content_type` for this dynamic group."""
+        """List the member objects of this dynamic group."""
         instance = get_object_or_404(self.queryset, pk=pk)
 
         # Retrieve the serializer for the content_type and paginate the results
         member_model_class = instance.content_type.model_class()
         member_serializer_class = get_serializer_for_model(member_model_class)
-        members = self.paginate_queryset(instance.members)
+        members = self.paginate_queryset(instance.members.restrict(request.user, "view"))
         member_serializer = member_serializer_class(members, many=True, context={"request": request})
         return self.get_paginated_response(member_serializer.data)
 

nautobot/extras/tests/test_api.py

@@ -17,6 +17,7 @@
 from nautobot.core.testing import APITestCase, APIViewTestCases
 from nautobot.core.testing.utils import disable_warnings
 from nautobot.core.utils.lookup import get_route_for_model
+from nautobot.core.utils.permissions import get_permission_for_model
 from nautobot.dcim.models import (
     Controller,
     Device,
@@ -768,7 +769,7 @@ def setUpTestData(cls):
 
         # Then the DynamicGroups.
         cls.content_type = ContentType.objects.get_for_model(Device)
-        cls.groups = cls.groups = [
+        cls.groups = [
             DynamicGroup.objects.create(
                 name="API DynamicGroup 1",
                 content_type=cls.content_type,
@@ -811,13 +812,34 @@ class DynamicGroupTest(DynamicGroupTestMixin, APIViewTestCases.APIViewTestCase):
     def test_get_members(self):
         """Test that the `/members/` API endpoint returns what is expected."""
         self.add_permissions("extras.view_dynamicgroup")
-        instance = DynamicGroup.objects.first()
+        instance = self.groups[0]
+        self.add_permissions(get_permission_for_model(instance.content_type.model_class(), "view"))
         member_count = instance.members.count()
         url = reverse("extras-api:dynamicgroup-members", kwargs={"pk": instance.pk})
         response = self.client.get(url, **self.header)
         self.assertHttpStatus(response, status.HTTP_200_OK)
         self.assertEqual(member_count, len(response.json()["results"]))
 
+    def test_get_members_with_constrained_permission(self):
+        """Test that the `/members/` API endpoint enforces permissions on the member model."""
+        self.add_permissions("extras.view_dynamicgroup")
+        instance = self.groups[0]
+        obj1 = instance.members.first()
+        obj_perm = ObjectPermission(
+            name="Test permission",
+            constraints={"pk__in": [obj1.pk]},
+            actions=["view"],
+        )
+        obj_perm.save()
+        obj_perm.users.add(self.user)
+        obj_perm.object_types.add(instance.content_type)
+
+        url = reverse("extras-api:dynamicgroup-members", kwargs={"pk": instance.pk})
+        response = self.client.get(url, **self.header)
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        self.assertEqual(len(response.json()["results"]), 1)
+        self.assertEqual(response.json()["results"][0]["id"], str(obj1.pk))
+
 
 class DynamicGroupMembershipTest(DynamicGroupTestMixin, APIViewTestCases.APIViewTestCase):
     model = DynamicGroupMembership

nautobot/extras/tests/test_views.py

@@ -17,6 +17,7 @@
 from nautobot.core.models.fields import slugify_dashes_to_underscores
 from nautobot.core.testing import extract_form_failures, extract_page_body, TestCase, ViewTestCases
 from nautobot.core.testing.utils import disable_warnings, post_data
+from nautobot.core.utils.permissions import get_permission_for_model
 from nautobot.dcim.models import (
     ConsolePort,
     Controller,
@@ -777,9 +778,11 @@ def setUpTestData(cls):
         content_type = ContentType.objects.get_for_model(Device)
 
         # DynamicGroup objects to test.
-        DynamicGroup.objects.create(name="DG 1", content_type=content_type)
-        DynamicGroup.objects.create(name="DG 2", content_type=content_type)
-        DynamicGroup.objects.create(name="DG 3", content_type=content_type)
+        cls.dynamic_groups = [
+            DynamicGroup.objects.create(name="DG 1", content_type=content_type),
+            DynamicGroup.objects.create(name="DG 2", content_type=content_type),
+            DynamicGroup.objects.create(name="DG 3", content_type=content_type),
+        ]
 
         cls.form_data = {
             "name": "new_dynamic_group",
@@ -792,6 +795,38 @@ def setUpTestData(cls):
             "dynamic_group_memberships-MAX_NUM_FORMS": "1000",
         }
 
+    def test_get_object_with_permission(self):
+        instance = self._get_queryset().first()
+        # Add view permissions for the group's members:
+        self.add_permissions(get_permission_for_model(instance.content_type.model_class(), "view"))
+
+        response = super().test_get_object_with_permission()
+
+        response_body = extract_page_body(response.content.decode(response.charset))
+        # Check that the "members" table in the detail view includes all appropriate member objects
+        for member in instance.members:
+            self.assertIn(str(member.pk), response_body)
+
+    def test_get_object_with_constrained_permission(self):
+        instance = self._get_queryset().first()
+        # Add view permission for one of the group's members but not the others:
+        member1, member2 = instance.members[:2]
+        obj_perm = ObjectPermission(
+            name="Members permission",
+            constraints={"pk": member1.pk},
+            actions=["view"],
+        )
+        obj_perm.save()
+        obj_perm.users.add(self.user)
+        obj_perm.object_types.add(instance.content_type)
+
+        response = super().test_get_object_with_constrained_permission()
+
+        response_body = extract_page_body(response.content.decode(response.charset))
+        # Check that the "members" table in the detail view includes all permitted member objects
+        self.assertIn(str(member1.pk), response_body)
+        self.assertNotIn(str(member2.pk), response_body)
+
     def test_get_object_dynamic_groups_anonymous(self):
         url = reverse("dcim:device_dynamicgroups", kwargs={"pk": Device.objects.first().pk})
         self.client.logout()
@@ -815,7 +850,6 @@ def test_get_object_dynamic_groups_with_permission(self):
         self.assertIn("DG 3", response_body, msg=response_body)
 
     def test_get_object_dynamic_groups_with_constrained_permission(self):
-        self.add_permissions("extras.view_dynamicgroup")
         obj_perm = ObjectPermission(
             name="View a device",
             constraints={"pk": Device.objects.first().pk},
@@ -824,12 +858,22 @@ def test_get_object_dynamic_groups_with_constrained_permission(self):
         obj_perm.save()
         obj_perm.users.add(self.user)
         obj_perm.object_types.add(ContentType.objects.get_for_model(Device))
+        obj_perm_2 = ObjectPermission(
+            name="View a Dynamic Group",
+            constraints={"pk": self.dynamic_groups[0].pk},
+            actions=["view"],
+        )
+        obj_perm_2.save()
+        obj_perm_2.users.add(self.user)
+        obj_perm_2.object_types.add(ContentType.objects.get_for_model(DynamicGroup))
 
         url = reverse("dcim:device_dynamicgroups", kwargs={"pk": Device.objects.first().pk})
         response = self.client.get(url)
         self.assertHttpStatus(response, 200)
         response_body = response.content.decode(response.charset)
         self.assertIn("DG 1", response_body, msg=response_body)
+        self.assertNotIn("DG 2", response_body, msg=response_body)
+        self.assertNotIn("DG 3", response_body, msg=response_body)
 
         url = reverse("dcim:device_dynamicgroups", kwargs={"pk": Device.objects.last().pk})
         response = self.client.get(url)

nautobot/extras/views.py

@@ -704,7 +704,7 @@ def get_extra_context(self, request, instance):
 
         if table_class is not None:
             # Members table (for display on Members nav tab)
-            members_table = table_class(instance.members, orderable=False)
+            members_table = table_class(instance.members.restrict(request.user, "view"), orderable=False)
             paginate = {
                 "paginator_class": EnhancedPaginator,
                 "per_page": get_paginate_count(request),
@@ -884,7 +884,9 @@ def get(self, request, model, **kwargs):
             obj = get_object_or_404(model, **kwargs)
 
         # Gather all dynamic groups for this object (and its related objects)
-        dynamicsgroups_table = tables.DynamicGroupTable(data=obj.dynamic_groups_cached, orderable=False)
+        dynamicsgroups_table = tables.DynamicGroupTable(
+            data=obj.dynamic_groups_cached.restrict(request.user, "view"), orderable=False
+        )
 
         # Apply the request context
         paginate = {