From d2e0b2628726ba6bafc2c647bb28ae535e5ff0f7 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Mon, 2 Oct 2017 13:48:48 -0400 Subject: [PATCH] allow deleting hosts and groups from inv src sublists --- awx/api/generics.py | 53 ++++++++++++++----- .../api/sub_list_destroy_api_view.md | 6 +++ awx/api/views.py | 6 ++- awx/main/tests/functional/api/test_generic.py | 31 +++++++++++ awx/main/tests/functional/conftest.py | 2 +- 5 files changed, 81 insertions(+), 17 deletions(-) create mode 100644 awx/api/templates/api/sub_list_destroy_api_view.md diff --git a/awx/api/generics.py b/awx/api/generics.py index 36176016b0..fcc48322ce 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -37,9 +37,10 @@ from awx.api.metadata import SublistAttachDetatchMetadata __all__ = ['APIView', 'GenericAPIView', 'ListAPIView', 'SimpleListAPIView', 'ListCreateAPIView', 'SubListAPIView', 'SubListCreateAPIView', + 'SubListDestroyAPIView', 'SubListCreateAttachDetachAPIView', 'RetrieveAPIView', 'RetrieveUpdateAPIView', 'RetrieveDestroyAPIView', - 'RetrieveUpdateDestroyAPIView', 'DestroyAPIView', + 'RetrieveUpdateDestroyAPIView', 'SubDetailAPIView', 'ResourceAccessList', 'ParentMixin', @@ -440,6 +441,41 @@ class SubListAPIView(ParentMixin, ListAPIView): return qs & sublist_qs +class DestroyAPIView(generics.DestroyAPIView): + + def has_delete_permission(self, obj): + return self.request.user.can_access(self.model, 'delete', obj) + + def perform_destroy(self, instance, check_permission=True): + if check_permission and not self.has_delete_permission(instance): + raise PermissionDenied() + super(DestroyAPIView, self).perform_destroy(instance) + + +class SubListDestroyAPIView(DestroyAPIView, SubListAPIView): + """ + Concrete view for deleting everything related by `relationship`. + """ + check_sub_obj_permission = True + + def destroy(self, request, *args, **kwargs): + instance_list = self.get_queryset() + if (not self.check_sub_obj_permission and + not request.user.can_access(self.parent_model, 'delete', self.get_parent_object())): + raise PermissionDenied() + self.perform_list_destroy(instance_list) + return Response(status=status.HTTP_204_NO_CONTENT) + + def perform_list_destroy(self, instance_list): + if self.check_sub_obj_permission: + # Check permissions for all before deleting, avoiding half-deleted lists + for instance in instance_list: + if self.has_delete_permission(instance): + raise PermissionDenied() + for instance in instance_list: + self.perform_destroy(instance, check_permission=False) + + class SubListCreateAPIView(SubListAPIView, ListCreateAPIView): # Base class for a sublist view that allows for creating subobjects # associated with the parent object. @@ -678,22 +714,11 @@ class RetrieveUpdateAPIView(RetrieveAPIView, generics.RetrieveUpdateAPIView): pass -class RetrieveDestroyAPIView(RetrieveAPIView, generics.RetrieveDestroyAPIView): - - def destroy(self, request, *args, **kwargs): - # somewhat lame that delete has to call it's own permissions check - obj = self.get_object() - if not request.user.can_access(self.model, 'delete', obj): - raise PermissionDenied() - obj.delete() - return Response(status=status.HTTP_204_NO_CONTENT) - - -class RetrieveUpdateDestroyAPIView(RetrieveUpdateAPIView, RetrieveDestroyAPIView): +class RetrieveDestroyAPIView(RetrieveAPIView, DestroyAPIView): pass -class DestroyAPIView(GenericAPIView, generics.DestroyAPIView): +class RetrieveUpdateDestroyAPIView(RetrieveUpdateAPIView, DestroyAPIView): pass diff --git a/awx/api/templates/api/sub_list_destroy_api_view.md b/awx/api/templates/api/sub_list_destroy_api_view.md new file mode 100644 index 0000000000..72654f88a0 --- /dev/null +++ b/awx/api/templates/api/sub_list_destroy_api_view.md @@ -0,0 +1,6 @@ +{% include "api/sub_list_create_api_view.md" %} + +# Delete all {{ model_verbose_name_plural }} of this {{ parent_model_verbose_name|title }}: + +Make a DELETE request to this resource to delete all {{ model_verbose_name_plural }} show in the list. +The {{ parent_model_verbose_name|title }} will not be deleted by this request. diff --git a/awx/api/views.py b/awx/api/views.py index 366ee29957..9763185c30 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2616,23 +2616,25 @@ class InventorySourceNotificationTemplatesSuccessList(InventorySourceNotificatio relationship = 'notification_templates_success' -class InventorySourceHostsList(SubListAPIView): +class InventorySourceHostsList(SubListDestroyAPIView): model = Host serializer_class = HostSerializer parent_model = InventorySource relationship = 'hosts' new_in_148 = True + check_sub_obj_permission = False capabilities_prefetch = ['inventory.admin'] -class InventorySourceGroupsList(SubListAPIView): +class InventorySourceGroupsList(SubListDestroyAPIView): model = Group serializer_class = GroupSerializer parent_model = InventorySource relationship = 'groups' new_in_148 = True + check_sub_obj_permission = False class InventorySourceUpdatesList(SubListAPIView): diff --git a/awx/main/tests/functional/api/test_generic.py b/awx/main/tests/functional/api/test_generic.py index 6b2d580d2d..4d68b43ead 100644 --- a/awx/main/tests/functional/api/test_generic.py +++ b/awx/main/tests/functional/api/test_generic.py @@ -60,3 +60,34 @@ def test_proxy_ip_whitelist(get, patch, admin): REMOTE_HOST='my.proxy.example.org', HTTP_X_FROM_THE_LOAD_BALANCER='some-actual-ip') assert middleware.environ['HTTP_X_FROM_THE_LOAD_BALANCER'] == 'some-actual-ip' + + +@pytest.mark.django_db +class TestDeleteViews: + def test_sublist_delete_permission_check(self, inventory_source, host, rando, delete): + inventory_source.hosts.add(host) + inventory_source.inventory.read_role.members.add(rando) + delete( + reverse( + 'api:inventory_source_hosts_list', + kwargs={'version': 'v2', 'pk': inventory_source.pk} + ), user=rando, expect=403 + ) + + def test_sublist_delete_functionality(self, inventory_source, host, rando, delete): + inventory_source.hosts.add(host) + inventory_source.inventory.admin_role.members.add(rando) + delete( + reverse( + 'api:inventory_source_hosts_list', + kwargs={'version': 'v2', 'pk': inventory_source.pk} + ), user=rando, expect=204 + ) + assert inventory_source.hosts.count() == 0 + + def test_destroy_permission_check(self, job_factory, system_auditor, delete): + job = job_factory() + resp = delete( + job.get_absolute_url(), user=system_auditor + ) + assert resp.status_code == 403 diff --git a/awx/main/tests/functional/conftest.py b/awx/main/tests/functional/conftest.py index 456d1a409e..2a4fea237f 100644 --- a/awx/main/tests/functional/conftest.py +++ b/awx/main/tests/functional/conftest.py @@ -377,7 +377,7 @@ def admin(user): @pytest.fixture def system_auditor(user): - u = user(False) + u = user('sys-auditor', False) Role.singleton('system_auditor').members.add(u) return u