Skip to content

Commit

Permalink
Avoid refetching resource instances inside permissions framework (#11542
Browse files Browse the repository at this point in the history 
)
  • Loading branch information
@jacobtylerwalls
jacobtylerwalls authored Oct 18, 2024
1 parent 656d006 commit 8ed87f3
Show file tree
Hide file tree
Showing 9 changed files with 196 additions and 95 deletions.
19 changes: 14 additions & 5 deletions arches/app/permissions/arches_default_allow.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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)
Expand Down
24 changes: 19 additions & 5 deletions arches/app/permissions/arches_default_deny.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down Expand Up @@ -295,17 +303,23 @@ 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
"""
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":
Expand Down
37 changes: 28 additions & 9 deletions arches/app/permissions/arches_permission_base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand All @@ -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":
Expand Down Expand Up @@ -340,17 +349,23 @@ 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
"""
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":
Expand All @@ -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
Expand All @@ -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":
Expand Down
26 changes: 14 additions & 12 deletions arches/app/utils/permission_backend.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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): ...
Expand All @@ -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): ...
Expand Down Expand Up @@ -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
)


Expand Down Expand Up @@ -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
)


Expand Down
87 changes: 47 additions & 40 deletions arches/app/views/api/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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:
#
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)


Expand Down
Loading

0 comments on commit 8ed87f3

Please sign in to comment.