From 62f34b920023b5a37cf8caee8be28d238ed27a4e Mon Sep 17 00:00:00 2001 From: Julien Barreau Date: Tue, 8 Oct 2024 16:09:48 +0200 Subject: [PATCH 1/2] fix(permissions): properly check permissions when dissociating or deleting related resources --- .../agent_toolkit/resources/collections/crud_related.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/agent_toolkit/forestadmin/agent_toolkit/resources/collections/crud_related.py b/src/agent_toolkit/forestadmin/agent_toolkit/resources/collections/crud_related.py index dacc6d5cb..29ee9e181 100644 --- a/src/agent_toolkit/forestadmin/agent_toolkit/resources/collections/crud_related.py +++ b/src/agent_toolkit/forestadmin/agent_toolkit/resources/collections/crud_related.py @@ -270,7 +270,6 @@ async def count(self, request: RequestRelationCollection) -> Response: @check_method(RequestMethod.DELETE) async def delete_list(self, request: RequestRelationCollection) -> Response: """delete and dissociate""" - await self.permission.can(request.user, request.collection, "delete") try: parent_ids = unpack_id(request.collection.schema, request.pks) except (FieldValidatorException, CollectionResourceException) as e: @@ -281,6 +280,11 @@ async def delete_list(self, request: RequestRelationCollection) -> Response: if request.query: delete_mode = bool(request.query.get("delete", False)) + if delete_mode is True: + await self.permission.can(request.user, request.foreign_collection, "delete") + else: + await self.permission.can(request.user, request.collection, "edit") + filter = await self.get_base_fk_filter(request) if ( From 68eb32347d091bf3614dc908244dfb2036cfb013 Mon Sep 17 00:00:00 2001 From: Julien Barreau Date: Tue, 8 Oct 2024 16:30:02 +0200 Subject: [PATCH 2/2] chore: add tests --- .../collections/test_crud_related.py | 59 +++++++++++++++++-- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/src/agent_toolkit/tests/resources/collections/test_crud_related.py b/src/agent_toolkit/tests/resources/collections/test_crud_related.py index 079d3e541..9c0461640 100644 --- a/src/agent_toolkit/tests/resources/collections/test_crud_related.py +++ b/src/agent_toolkit/tests/resources/collections/test_crud_related.py @@ -1254,7 +1254,7 @@ def test_delete_list(self, mock_mach_id: Mock): ) as fake_delete_one_to_many: response = self.loop.run_until_complete(self.crud_related_resource.delete_list(request)) fake_delete_one_to_many.assert_awaited() - self.permission_service.can.assert_any_await(request.user, request.collection, "delete") + self.permission_service.can.assert_any_await(request.user, request.collection, "edit") self.permission_service.can.reset_mock() assert response.status == 204 @@ -1275,7 +1275,7 @@ def test_delete_list(self, mock_mach_id: Mock): ) as fake_delete_many_to_many: response = self.loop.run_until_complete(self.crud_related_resource.delete_list(request)) fake_delete_many_to_many.assert_awaited() - self.permission_service.can.assert_any_await(request.user, request.collection, "delete") + self.permission_service.can.assert_any_await(request.user, request.collection, "edit") self.permission_service.can.reset_mock() assert response.status == 204 @@ -1303,7 +1303,7 @@ def test_delete_list_error(self, mock_mach_id: Mock): None, # user ) response = self.loop.run_until_complete(crud_related_resource.delete_list(request)) - self.permission_service.can.assert_any_await(request.user, request.collection, "delete") + self.permission_service.can.assert_not_awaited() self.permission_service.can.reset_mock() assert response.status == 500 response_content = json.loads(response.body) @@ -1331,7 +1331,7 @@ def test_delete_list_error(self, mock_mach_id: Mock): ) as fake_delete_many_to_many: response = self.loop.run_until_complete(crud_related_resource.delete_list(request)) fake_delete_many_to_many.assert_awaited() - self.permission_service.can.assert_any_await(request.user, request.collection, "delete") + self.permission_service.can.assert_any_await(request.user, request.collection, "edit") self.permission_service.can.reset_mock() assert response.status == 500 response_content = json.loads(response.body) @@ -1358,7 +1358,7 @@ def test_delete_list_error(self, mock_mach_id: Mock): None, # user ) response = self.loop.run_until_complete(crud_related_resource.delete_list(request)) - self.permission_service.can.assert_any_await(request.user, request.collection, "delete") + self.permission_service.can.assert_any_await(request.user, request.collection, "edit") self.permission_service.can.reset_mock() assert response.status == 500 response_content = json.loads(response.body) @@ -1368,6 +1368,55 @@ def test_delete_list_error(self, mock_mach_id: Mock): "status": 500, } + def test_delete_list_should_check_delete_permission_when_delete_flag_is_set(self): + query_get_params = { + "collection_name": "customer", + "relation_name": "order", + "timezone": "Europe/Paris", + "fields[order]": "id,cost", + "pks": "2", # customer id + "delete": True, + } + request = RequestRelationCollection( + RequestMethod.DELETE, + *self.mk_request_customer_order_one_to_many(), + {"data": [{"id": "201", "type": "order"}]}, + query_get_params, + {}, + None, + ) + with patch.object( + self.crud_related_resource, "_delete_one_to_many", new_callable=AsyncMock + ) as fake_delete_one_to_many: + self.loop.run_until_complete(self.crud_related_resource.delete_list(request)) + fake_delete_one_to_many.assert_awaited() + self.permission_service.can.assert_any_await(request.user, request.foreign_collection, "delete") + self.permission_service.can.reset_mock() + + def test_delete_list_should_check_edit_permission_when_delete_flag_is_not_set(self): + query_get_params = { + "collection_name": "customer", + "relation_name": "order", + "timezone": "Europe/Paris", + "fields[order]": "id,cost", + "pks": "2", # customer id + } + request = RequestRelationCollection( + RequestMethod.DELETE, + *self.mk_request_customer_order_one_to_many(), + {"data": [{"id": "201", "type": "order"}]}, + query_get_params, + {}, + None, + ) + with patch.object( + self.crud_related_resource, "_delete_one_to_many", new_callable=AsyncMock + ) as fake_delete_one_to_many: + self.loop.run_until_complete(self.crud_related_resource.delete_list(request)) + fake_delete_one_to_many.assert_awaited() + self.permission_service.can.assert_any_await(request.user, request.collection, "edit") + self.permission_service.can.reset_mock() + # _associate_one_to_many def test_associate_one_to_many(self): crud_related_resource = CrudRelatedResource(