From b4d60832ad5a01494cbf23e8a44c4cef63bf4f24 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Mon, 27 Jan 2025 13:58:35 -0500 Subject: [PATCH 1/2] Fix user_can_* checks handling of resource kwarg Follow-up to c86d4d9. --- .../app/permissions/arches_default_allow.py | 10 ++++---- arches/app/permissions/arches_default_deny.py | 2 +- .../app/permissions/arches_permission_base.py | 6 ++--- .../permissions_arches_default_allow_tests.py | 1 - tests/views/api/test_permissions.py | 25 ++++++++++++++++++- 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/arches/app/permissions/arches_default_allow.py b/arches/app/permissions/arches_default_allow.py index b61996e6e9f..17a11528b24 100644 --- a/arches/app/permissions/arches_default_allow.py +++ b/arches/app/permissions/arches_default_allow.py @@ -261,11 +261,6 @@ def check_resource_instance_permissions( """ result = ResourceInstancePermissions() try: - if resourceid == settings.SYSTEM_SETTINGS_RESOURCE_ID: - if not user.groups.filter(name="System Administrator").exists(): - result["permitted"] = False - return result - if not resource: resource = ResourceInstance.objects.select_related( "resource_instance_lifecycle_state" @@ -274,6 +269,11 @@ def check_resource_instance_permissions( raise ValueError("resourceid and resource are mutually incompatible") result["resource"] = resource + if str(resource.pk) == settings.SYSTEM_SETTINGS_RESOURCE_ID: + if not user.groups.filter(name="System Administrator").exists(): + result["permitted"] = False + return result + all_perms = self.get_perms(user, resource) if len(all_perms) == 0: # no permissions assigned. permission implied diff --git a/arches/app/permissions/arches_default_deny.py b/arches/app/permissions/arches_default_deny.py index 741e305c93e..55aa6f77e96 100644 --- a/arches/app/permissions/arches_default_deny.py +++ b/arches/app/permissions/arches_default_deny.py @@ -119,7 +119,7 @@ def check_resource_instance_permissions( resource = ResourceInstance.objects.get(resourceinstanceid=resourceid) elif resourceid: raise ValueError("resourceid and resource are mutually incompatible") - if resourceid == settings.SYSTEM_SETTINGS_RESOURCE_ID: + if str(resource.pk) == settings.SYSTEM_SETTINGS_RESOURCE_ID: result["resource"] = resource if not user.groups.filter(name="System Administrator").exists(): result["permitted"] = False diff --git a/arches/app/permissions/arches_permission_base.py b/arches/app/permissions/arches_permission_base.py index 32243baa8bf..0907afd4c01 100644 --- a/arches/app/permissions/arches_permission_base.py +++ b/arches/app/permissions/arches_permission_base.py @@ -310,7 +310,7 @@ def user_can_read_resource( if user.is_authenticated: if user.is_superuser: return True - if resourceid is not None and resourceid != "": + if resourceid or resource: result = self.check_resource_instance_permissions( user, resourceid, "view_resourceinstance", resource=resource ) @@ -363,7 +363,7 @@ def user_can_edit_resource( if user.is_authenticated: if user.is_superuser: return True - if resourceid: + if resourceid or resource: result = self.check_resource_instance_permissions( user, resourceid, "change_resourceinstance", resource=resource ) @@ -399,7 +399,7 @@ def user_can_delete_resource( if user.is_authenticated: if user.is_superuser: return True - if resourceid: + if resourceid or resource: result = self.check_resource_instance_permissions( user, resourceid, "delete_resourceinstance", resource=resource ) diff --git a/tests/permissions/permissions_arches_default_allow_tests.py b/tests/permissions/permissions_arches_default_allow_tests.py index 6b0a3e669a0..a8dcf7b0a88 100644 --- a/tests/permissions/permissions_arches_default_allow_tests.py +++ b/tests/permissions/permissions_arches_default_allow_tests.py @@ -15,7 +15,6 @@ from unittest.mock import MagicMock, Mock, patch -from django.test import override_settings from arches.app.models.resource import Resource from tests.permissions.base_permissions_framework_test import ( ArchesPermissionFrameworkTestCase, diff --git a/tests/views/api/test_permissions.py b/tests/views/api/test_permissions.py index 4002b29b580..a4c2eda6593 100644 --- a/tests/views/api/test_permissions.py +++ b/tests/views/api/test_permissions.py @@ -1,3 +1,5 @@ +from django.conf import settings +from django.contrib.auth.models import Group, User from django.db import connection from django.test import TestCase from django.test.utils import CaptureQueriesContext @@ -15,9 +17,14 @@ class InstancePermissionsAPITest(TestCase): def setUpTestData(cls): cls.graph = Graph.new( name="INSTANCE_PERMISSIONS_TEST_GRAPH", - is_resource=True, + is_resource=False, # creates a nodegroup, will undo this below. author="ARCHES TEST", ) + cls.graph.isresource = True + cls.graph.resource_instance_lifecycle_id = ( + settings.DEFAULT_RESOURCE_INSTANCE_LIFECYCLE_ID + ) + cls.graph.save(validate=False) def test_get(self): resource = ResourceInstance.objects.create(graph=self.graph) @@ -33,3 +40,19 @@ def test_get(self): self.assertEqual( response.content.decode(), '{"delete": false, "edit": false, "read": false}' ) + + def test_get_with_resource_editor_role(self): + resource = ResourceInstance.objects.create(graph=self.graph) + editor_group = Group.objects.get(name="Resource Editor") + test_user = User.objects.create() + test_user.groups.add(editor_group) + self.client.force_login(test_user) + + response = self.client.get( + reverse("api_instance_permissions"), + QUERY_STRING=f"resourceinstanceid={resource.pk}", + ) + + self.assertEqual( + response.content.decode(), '{"delete": true, "edit": true, "read": true}' + ) From 7c1cf73f4795285e944a495bc6db423e8a4b0f83 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Mon, 27 Jan 2025 15:32:15 -0500 Subject: [PATCH 2/2] Update test result --- tests/views/api/test_permissions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/views/api/test_permissions.py b/tests/views/api/test_permissions.py index a4c2eda6593..eb297abd4da 100644 --- a/tests/views/api/test_permissions.py +++ b/tests/views/api/test_permissions.py @@ -26,7 +26,7 @@ def setUpTestData(cls): ) cls.graph.save(validate=False) - def test_get(self): + def test_get_with_anonymous_user(self): resource = ResourceInstance.objects.create(graph=self.graph) with CaptureQueriesContext(connection) as queries: response = self.client.get( @@ -38,7 +38,7 @@ def test_get(self): ] self.assertEqual(len(resource_selects), 1, list(queries)) self.assertEqual( - response.content.decode(), '{"delete": false, "edit": false, "read": false}' + response.content.decode(), '{"delete": true, "edit": true, "read": true}' ) def test_get_with_resource_editor_role(self):