From 8ed87f361c63b1e432d048c6f3b3b7c7f72d8a57 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Fri, 18 Oct 2024 08:58:07 -0700 Subject: [PATCH] Avoid refetching resource instances inside permissions framework (#11542) --- .../app/permissions/arches_default_allow.py | 19 ++-- arches/app/permissions/arches_default_deny.py | 24 +++-- .../app/permissions/arches_permission_base.py | 37 ++++++-- arches/app/utils/permission_backend.py | 26 +++--- arches/app/views/api/__init__.py | 87 ++++++++++--------- arches/app/views/resource.py | 10 ++- releases/8.0.0.md | 5 ++ tests/views/api/test_permissions.py | 35 ++++++++ tests/views/api/test_resources.py | 48 +++++----- 9 files changed, 196 insertions(+), 95 deletions(-) create mode 100644 tests/views/api/test_permissions.py diff --git a/arches/app/permissions/arches_default_allow.py b/arches/app/permissions/arches_default_allow.py index 262eb282a59..b61996e6e9f 100644 --- a/arches/app/permissions/arches_default_allow.py +++ b/arches/app/permissions/arches_default_allow.py @@ -241,7 +241,12 @@ def get_restricted_instances( return restricted_ids def check_resource_instance_permissions( - self, user: User, resourceid: str, permission: str + self, + user: User, + resourceid: str | None, + permission: str, + *, + resource: ResourceInstance | None = None, ) -> ResourceInstancePermissions: """ Checks if a user has permission to access a resource instance @@ -251,6 +256,8 @@ def check_resource_instance_permissions( resourceid -- the id of the resource permission -- the permission codename (e.g. 'view_resourceinstance') for which to check + Optional arguments: + resource -- provide an instance to avoid having to fetch it by ID. """ result = ResourceInstancePermissions() try: @@ -259,10 +266,12 @@ def check_resource_instance_permissions( result["permitted"] = False return result - resource = ResourceInstance.objects.select_related( - "resource_instance_lifecycle_state" - ).get(resourceinstanceid=resourceid) - + if not resource: + resource = ResourceInstance.objects.select_related( + "resource_instance_lifecycle_state" + ).get(resourceinstanceid=resourceid) + elif resourceid: + raise ValueError("resourceid and resource are mutually incompatible") result["resource"] = resource all_perms = self.get_perms(user, resource) diff --git a/arches/app/permissions/arches_default_deny.py b/arches/app/permissions/arches_default_deny.py index 76a1b9127f4..741e305c93e 100644 --- a/arches/app/permissions/arches_default_deny.py +++ b/arches/app/permissions/arches_default_deny.py @@ -106,11 +106,19 @@ def get_allowed_instances( return restricted_ids def check_resource_instance_permissions( - self, user: User, resourceid: str, permission: str + self, + user: User, + resourceid: str | None, + permission: str, + *, + resource: ResourceInstance | None = None, ) -> ResourceInstancePermissions: result = ResourceInstancePermissions() - resource = ResourceInstance.objects.get(resourceinstanceid=resourceid) + if not resource: + resource = ResourceInstance.objects.get(resourceinstanceid=resourceid) + elif resourceid: + raise ValueError("resourceid and resource are mutually incompatible") if resourceid == settings.SYSTEM_SETTINGS_RESOURCE_ID: result["resource"] = resource if not user.groups.filter(name="System Administrator").exists(): @@ -295,7 +303,13 @@ def get_search_ui_permissions( return result - def user_can_read_resource(self, user: User, resourceid: str | None = None) -> bool: + def user_can_read_resource( + self, + user: User, + resourceid: str | None = None, + *, + resource: ResourceInstance | None = None, + ) -> bool: """ Requires that a user be able to read an instance and read a single nodegroup of a resource @@ -303,9 +317,9 @@ def user_can_read_resource(self, user: User, resourceid: str | None = None) -> b if user.is_authenticated: if user.is_superuser: return True - if resourceid is not None and resourceid != "": + if resourceid: result = self.check_resource_instance_permissions( - user, resourceid, "view_resourceinstance" + user, resourceid, "view_resourceinstance", resource=resource ) if result is not None: if result["permitted"] == "unknown": diff --git a/arches/app/permissions/arches_permission_base.py b/arches/app/permissions/arches_permission_base.py index 66947dcfa1c..388558db4be 100644 --- a/arches/app/permissions/arches_permission_base.py +++ b/arches/app/permissions/arches_permission_base.py @@ -207,7 +207,12 @@ def has_group_perm(self, group, perm, obj): ... @abstractmethod def check_resource_instance_permissions( - self, user: User, resourceid: str, permission: str + self, + user: User, + resourceid: str, + permission: str, + *, + resource: ResourceInstance | None, ): ... def get_groups_with_permission_for_object( @@ -292,7 +297,11 @@ def user_has_resource_model_permissions( return nodes.exists() def user_can_read_resource( - self, user: User, resourceid: str | None = None + self, + user: User, + resourceid: str | None = None, + *, + resource: ResourceInstance | None = None, ) -> bool | None: """ Requires that a user be able to read an instance and read a single nodegroup of a resource @@ -303,7 +312,7 @@ def user_can_read_resource( return True if resourceid is not None and resourceid != "": result = self.check_resource_instance_permissions( - user, resourceid, "view_resourceinstance" + user, resourceid, "view_resourceinstance", resource=resource ) if result is not None: if result["permitted"] == "unknown": @@ -340,7 +349,13 @@ def get_resource_types_by_perm( return list(str(graph) for graph in graphs) - def user_can_edit_resource(self, user: User, resourceid: str | None = None) -> bool: + def user_can_edit_resource( + self, + user: User, + resourceid: str | None = None, + *, + resource: ResourceInstance | None = None, + ) -> bool: """ Requires that a user be able to edit an instance and delete a single nodegroup of a resource @@ -348,9 +363,9 @@ def user_can_edit_resource(self, user: User, resourceid: str | None = None) -> b if user.is_authenticated: if user.is_superuser: return True - if resourceid is not None and resourceid != "": + if resourceid: result = self.check_resource_instance_permissions( - user, resourceid, "change_resourceinstance" + user, resourceid, "change_resourceinstance", resource=resource ) if result is not None: if result["permitted"] == "unknown": @@ -371,7 +386,11 @@ def user_can_edit_resource(self, user: User, resourceid: str | None = None) -> b return False def user_can_delete_resource( - self, user: User, resourceid: str | None = None + self, + user: User, + resourceid: str | None = None, + *, + resource: ResourceInstance | None = None, ) -> bool | None: """ Requires that a user be permitted to delete an instance @@ -380,9 +399,9 @@ def user_can_delete_resource( if user.is_authenticated: if user.is_superuser: return True - if resourceid is not None and resourceid != "": + if resourceid: result = self.check_resource_instance_permissions( - user, resourceid, "delete_resourceinstance" + user, resourceid, "delete_resourceinstance", resource=resource ) if result is not None: if result["permitted"] == "unknown": diff --git a/arches/app/utils/permission_backend.py b/arches/app/utils/permission_backend.py index 63cc7f13c2e..09751ba38f2 100644 --- a/arches/app/utils/permission_backend.py +++ b/arches/app/utils/permission_backend.py @@ -77,7 +77,9 @@ def get_groups_with_permission_for_object(self, perm, obj): ... def get_users_with_permission_for_object(self, perm, obj): ... @abstractmethod - def check_resource_instance_permissions(self, user, resourceid, permission): ... + def check_resource_instance_permissions( + self, user, resourceid, permission, *, resource=None + ): ... @abstractmethod def get_nodegroups_by_perm(self, user, perms, any_perm=True): ... @@ -98,13 +100,13 @@ def process_new_user(self, instance, created): ... def user_has_resource_model_permissions(self, user, perms, resource): ... @abstractmethod - def user_can_read_resource(self, user, resourceid=None): ... + def user_can_read_resource(self, user, resourceid=None, *, resource=None): ... @abstractmethod - def user_can_edit_resource(self, user, resourceid=None): ... + def user_can_edit_resource(self, user, resourceid=None, *, resource=None): ... @abstractmethod - def user_can_delete_resource(self, user, resourceid=None): ... + def user_can_delete_resource(self, user, resourceid=None, *, resource=None): ... @abstractmethod def user_can_read_concepts(self, user): ... @@ -217,9 +219,9 @@ def get_users_with_permission_for_object(perm, obj): return _get_permission_framework().get_users_with_permission_for_object(perm, obj) -def check_resource_instance_permissions(user, resourceid, permission): +def check_resource_instance_permissions(user, resourceid, permission, *, resource=None): return _get_permission_framework().check_resource_instance_permissions( - user, resourceid, permission + user, resourceid, permission, resource=resource ) @@ -309,21 +311,21 @@ def user_has_resource_model_permissions(user, perms, resource): ) -def user_can_read_resource(user, resourceid=None): +def user_can_read_resource(user, resourceid=None, *, resource=None): return _get_permission_framework().user_can_read_resource( - user, resourceid=resourceid + user, resourceid=resourceid, resource=resource ) -def user_can_edit_resource(user, resourceid=None): +def user_can_edit_resource(user, resourceid=None, *, resource=None): return _get_permission_framework().user_can_edit_resource( - user, resourceid=resourceid + user, resourceid=resourceid, resource=resource ) -def user_can_delete_resource(user, resourceid=None): +def user_can_delete_resource(user, resourceid=None, *, resource=None): return _get_permission_framework().user_can_delete_resource( - user, resourceid=resourceid + user, resourceid=resourceid, resource=resource ) diff --git a/arches/app/views/api/__init__.py b/arches/app/views/api/__init__.py index c7e2b6e4641..5a40f5f7054 100644 --- a/arches/app/views/api/__init__.py +++ b/arches/app/views/api/__init__.py @@ -502,7 +502,24 @@ class Resources(APIBase): # }] def get(self, request, resourceid=None, slug=None, graphid=None): - if not user_can_read_resource(user=request.user, resourceid=resourceid): + try: + resource = ( + Resource.objects.filter(pk=resourceid) + .select_related( + "graph", + "resource_instance_lifecycle_state", + ) + .get() + ) + except Resource.DoesNotExist as dne: + logger.error( + _("The specified resource '{0}' does not exist. Export failed.").format( + resourceid + ) + ) + return JSONErrorResponse(message=dne.args[0], status=HTTPStatus.NOT_FOUND) + + if not user_can_read_resource(user=request.user, resource=resource): return JSONResponse(status=403) allowed_formats = ["json", "json-ld", "arches-json"] @@ -526,8 +543,6 @@ def get(self, request, resourceid=None, slug=None, graphid=None): if resourceid: if format == "json": - resource = Resource.objects.get(pk=resourceid) - version = request.GET.get("v", None) compact = bool( request.GET.get("compact", "true").lower() == "true" @@ -564,8 +579,7 @@ def get(self, request, resourceid=None, slug=None, graphid=None): } elif format == "arches-json": - out = Resource.objects.get(pk=resourceid) - + out = resource include_tiles = bool( request.GET.get("includetiles", "true").lower() == "true" ) # default True @@ -574,31 +588,20 @@ def get(self, request, resourceid=None, slug=None, graphid=None): out.load_tiles(user, perm) elif format == "json-ld": - try: - resource = models.ResourceInstance.objects.select_related( - "graph" - ).get(pk=resourceid) - if not resource.graph.ontology_id: - return JSONErrorResponse( - message=_( - "The graph '{0}' does not have an ontology. JSON-LD requires one." - ).format(resource.graph.name), - status=400, - ) - exporter = ResourceExporter(format=format) - output = exporter.writer.write_resources( - resourceinstanceids=[resourceid], - indent=indent, - user=request.user, - ) - out = output[0]["outputfile"].getvalue() - except models.ResourceInstance.DoesNotExist: - logger.error( - _( - "The specified resource '{0}' does not exist. JSON-LD export failed." - ).format(resourceid) + if not resource.graph.ontology_id: + return JSONErrorResponse( + message=_( + "The graph '{0}' does not have an ontology. JSON-LD requires one." + ).format(resource.graph.name), + status=400, ) - return JSONResponse(status=404) + exporter = ResourceExporter(format=format) + output = exporter.writer.write_resources( + resourceinstanceids=[resourceid], + indent=indent, + user=request.user, + ) + out = output[0]["outputfile"].getvalue() else: # @@ -685,7 +688,7 @@ def put(self, request, resourceid, slug=None, graphid=None): ) if not user_can_edit_resource(user=request.user, resourceid=resourceid): - return JSONResponse(status=403) + return JSONErrorResponse(status=403) else: with transaction.atomic(): try: @@ -885,14 +888,14 @@ def post(self, request, resourceid=None, slug=None, graphid=None): ) def delete(self, request, resourceid, slug=None, graphid=None): + try: + resource_instance = Resource.objects.get(pk=resourceid) + except Resource.DoesNotExist as dne: + return JSONErrorResponse(message=dne.args[0], status=404) if user_can_edit_resource( - user=request.user, resourceid=resourceid - ) and user_can_delete_resource(user=request.user, resourceid=resourceid): - try: - resource_instance = Resource.objects.get(pk=resourceid) - resource_instance.delete() - except models.ResourceInstance.DoesNotExist: - return JSONResponse(status=404) + user=request.user, resource=resource_instance + ) and user_can_delete_resource(user=request.user, resource=resource_instance): + resource_instance.delete() else: return JSONResponse(status=500) @@ -1826,9 +1829,13 @@ def get(self, request): user = request.user result = {} resourceinstanceid = request.GET.get("resourceinstanceid") - result["read"] = user_can_read_resource(user, resourceinstanceid) - result["edit"] = user_can_edit_resource(user, resourceinstanceid) - result["delete"] = user_can_delete_resource(user, resourceinstanceid) + try: + resource = models.ResourceInstance.objects.get(pk=resourceinstanceid) + except models.ResourceInstance.DoesNotExist as dne: + return JSONErrorResponse(message=dne.args[0], status=HTTPStatus.NOT_FOUND) + result["read"] = user_can_read_resource(user, resource=resource) + result["edit"] = user_can_edit_resource(user, resource=resource) + result["delete"] = user_can_delete_resource(user, resource=resource) return JSONResponse(result) diff --git a/arches/app/views/resource.py b/arches/app/views/resource.py index 5a0f4947e78..21a17eae47a 100644 --- a/arches/app/views/resource.py +++ b/arches/app/views/resource.py @@ -945,9 +945,11 @@ def get(self, request, resourceid=None): @method_decorator(can_read_resource_instance, name="dispatch") class ResourceReportView(MapBaseManagerView): def get(self, request, resourceid=None): - resource = Resource.objects.only( - "graph_id", "resource_instance_lifecycle_state" - ).get(pk=resourceid) + resource = ( + models.ResourceInstance.objects.filter(pk=resourceid) + .select_related("resource_instance_lifecycle_state") + .get() + ) graph = Graph.objects.get(graphid=resource.graph_id) graph_has_different_publication = bool( resource.graph_publication_id != graph.publication_id @@ -985,7 +987,7 @@ def get(self, request, resourceid=None): ) context["user_can_edit_resource"] = user_can_edit_resource( - request.user, resourceid=resourceid + request.user, resource=resource ) context["resource_instance_lifecycle_state_permits_editing"] = ( diff --git a/releases/8.0.0.md b/releases/8.0.0.md index d45393ca11e..d8e203ec284 100644 --- a/releases/8.0.0.md +++ b/releases/8.0.0.md @@ -9,6 +9,11 @@ Arches 8.0.0 Release Notes ### Performance improvements - Improve indexing and bulk deletion performance [#11382](https://github.com/archesproject/arches/issues/11382) +- The following methods or functions in the permissions framework now take an optional ``resource`` keyword argument to avoid refetching an instance that a caller may already have: + - `user_can_read_resource()` + - `user_can_edit_resource()` + - `user_can_delete_resource()` + - `check_resource_instance_permissions()` ### Additional highlights diff --git a/tests/views/api/test_permissions.py b/tests/views/api/test_permissions.py new file mode 100644 index 00000000000..4002b29b580 --- /dev/null +++ b/tests/views/api/test_permissions.py @@ -0,0 +1,35 @@ +from django.db import connection +from django.test import TestCase +from django.test.utils import CaptureQueriesContext +from django.urls import reverse + +from arches.app.models.graph import Graph +from arches.app.models.models import ResourceInstance + +# these tests can be run from the command line via +# python manage.py test tests.views.api.test_permissions --settings="tests.test_settings" + + +class InstancePermissionsAPITest(TestCase): + @classmethod + def setUpTestData(cls): + cls.graph = Graph.new( + name="INSTANCE_PERMISSIONS_TEST_GRAPH", + is_resource=True, + author="ARCHES TEST", + ) + + def test_get(self): + resource = ResourceInstance.objects.create(graph=self.graph) + with CaptureQueriesContext(connection) as queries: + response = self.client.get( + reverse("api_instance_permissions"), + QUERY_STRING=f"resourceinstanceid={resource.pk}", + ) + resource_selects = [ + q for q in queries if q["sql"].startswith('SELECT "resource_instances"') + ] + self.assertEqual(len(resource_selects), 1, list(queries)) + self.assertEqual( + response.content.decode(), '{"delete": false, "edit": false, "read": false}' + ) diff --git a/tests/views/api/test_resources.py b/tests/views/api/test_resources.py index adf08a9e3bc..9ee85774d56 100644 --- a/tests/views/api/test_resources.py +++ b/tests/views/api/test_resources.py @@ -356,18 +356,22 @@ def test_api_resources_archesjson(self): # ==PUT============================================================================================= # ==Act : GET confirmation that resource does not exist in database================================= - with self.assertRaises(models.ResourceInstance.DoesNotExist) as context: - with self.assertLogs("django.request", level="ERROR"): - resp_get = self.client.get( - reverse( - "resources", - kwargs={"resourceid": "075957c4-d97f-4986-8d27-c32b6dec8e62"}, - ) - + "?format=arches-json" + with ( + self.assertLogs("django.request", level="WARNING"), + self.assertLogs("arches.app.views.api", level="ERROR"), + ): + resp_get = self.client.get( + reverse( + "resources", + kwargs={"resourceid": "075957c4-d97f-4986-8d27-c32b6dec8e62"}, ) + + "?format=arches-json" + ) # ==Assert========================================================================================== - self.assertTrue( - "Resource matching query does not exist." in str(context.exception) + self.assertContains( + resp_get, + "Resource matching query does not exist.", + status_code=HTTPStatus.NOT_FOUND, ) # Check exception message. # ================================================================================================== @@ -488,18 +492,22 @@ def test_api_resources_archesjson(self): # ================================================================================================== # ==Act : GET confirmation that resource does not exist in database================================= - with self.assertRaises(models.ResourceInstance.DoesNotExist) as context_del: - with self.assertLogs("django.request", level="ERROR"): - resp_get_deleted = self.client.get( - reverse( - "resources", - kwargs={"resourceid": my_resource_resourceinstanceid}, - ) - + "?format=arches-json" + with ( + self.assertLogs("django.request", level="WARNING"), + self.assertLogs("arches.app.views.api", level="ERROR"), + ): + resp_get_deleted = self.client.get( + reverse( + "resources", + kwargs={"resourceid": my_resource_resourceinstanceid}, ) + + "?format=arches-json" + ) # ==Assert========================================================================================== - self.assertTrue( - "Resource matching query does not exist." in str(context_del.exception) + self.assertContains( + resp_get_deleted, + "Resource matching query does not exist.", + status_code=HTTPStatus.NOT_FOUND, ) # Check exception message. # ==================================================================================================