diff --git a/changes/5757.security b/changes/5757.security new file mode 100644 index 00000000000..df0fce05d68 --- /dev/null +++ b/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)). diff --git a/nautobot/core/testing/views.py b/nautobot/core/testing/views.py index e2e33cb7f35..0e2885cb461 100644 --- a/nautobot/core/testing/views.py +++ b/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() diff --git a/nautobot/extras/api/views.py b/nautobot/extras/api/views.py index 9bcfac020ae..8f196736b36 100644 --- a/nautobot/extras/api/views.py +++ b/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) diff --git a/nautobot/extras/tests/test_api.py b/nautobot/extras/tests/test_api.py index 4523d92de56..77647b37686 100644 --- a/nautobot/extras/tests/test_api.py +++ b/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 diff --git a/nautobot/extras/tests/test_views.py b/nautobot/extras/tests/test_views.py index d5e97176b95..e9857298570 100644 --- a/nautobot/extras/tests/test_views.py +++ b/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) diff --git a/nautobot/extras/views.py b/nautobot/extras/views.py index 330adbf7ebf..b69f99fb426 100644 --- a/nautobot/extras/views.py +++ b/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 = {