From 2d6ac6258d9e23ede068b1900ae0e65e3f849021 Mon Sep 17 00:00:00 2001 From: Roman Donchenko Date: Fri, 30 Aug 2024 12:36:27 +0300 Subject: [PATCH] Don't check permissions for the target resource when canceling requests (#8369) IMO, these checks are not very useful. The permission logic for requests already checks that the request is being canceled by the same user that created it. Therefore, these additional checks can only fail if a user creates a request for some action, loses the permissions to do the same action again, and then tries to cancel the request. But cancelling a request does not do anything to the target resource (in fact, it _prevents_ some future actions from taking place), so I really don't see why this shouldn't be allowed. In addition, these checks create some problems: * If the creator of the request is no longer able to cancel it, we now have a request that _nobody_ is allowed to cancel. That seems wrong. * To implement these checks, `RequestPermission` has to know which actions require which permissions. This creates code duplication between it and the other permission classes. It also causes a dependency on those classes, which could create problems if we want to use the request API for actions from the Enterprise version. --- .../20240828_053041_roman_rm_extra_checks.md | 5 +++ cvat/apps/engine/permissions.py | 38 +------------------ 2 files changed, 6 insertions(+), 37 deletions(-) create mode 100644 changelog.d/20240828_053041_roman_rm_extra_checks.md diff --git a/changelog.d/20240828_053041_roman_rm_extra_checks.md b/changelog.d/20240828_053041_roman_rm_extra_checks.md new file mode 100644 index 000000000000..74337256b1f4 --- /dev/null +++ b/changelog.d/20240828_053041_roman_rm_extra_checks.md @@ -0,0 +1,5 @@ +### Changed + +- When cancelling a request, a user is no longer required to have + permissions to perform the original action + () diff --git a/cvat/apps/engine/permissions.py b/cvat/apps/engine/permissions.py index efa7d3f7b99f..92d1b107b747 100644 --- a/cvat/apps/engine/permissions.py +++ b/cvat/apps/engine/permissions.py @@ -9,7 +9,7 @@ from django.shortcuts import get_object_or_404 from django.conf import settings -from rest_framework.exceptions import ValidationError, PermissionDenied, NotFound +from rest_framework.exceptions import ValidationError, PermissionDenied from rq.job import Job as RQJob from cvat.apps.engine.rq_job_handler import is_rq_job_owner @@ -1219,42 +1219,6 @@ def create(cls, request, view, obj: Optional[RQJob], iam_context: Dict): permissions = [] if view.basename == 'request': for scope in cls.get_scopes(request, view, obj): - if scope == cls.Scopes.CANCEL: - parsed_rq_id = obj.parsed_rq_id - - permission_class, resource_scope = { - ('import', 'project', 'dataset'): (ProjectPermission, ProjectPermission.Scopes.IMPORT_DATASET), - ('import', 'project', 'backup'): (ProjectPermission, ProjectPermission.Scopes.IMPORT_BACKUP), - ('import', 'task', 'annotations'): (TaskPermission, TaskPermission.Scopes.IMPORT_ANNOTATIONS), - ('import', 'task', 'backup'): (TaskPermission, TaskPermission.Scopes.IMPORT_BACKUP), - ('import', 'job', 'annotations'): (JobPermission, JobPermission.Scopes.IMPORT_ANNOTATIONS), - ('create', 'task', None): (TaskPermission, TaskPermission.Scopes.VIEW), - ('export', 'project', 'annotations'): (ProjectPermission, ProjectPermission.Scopes.EXPORT_ANNOTATIONS), - ('export', 'project', 'dataset'): (ProjectPermission, ProjectPermission.Scopes.EXPORT_DATASET), - ('export', 'project', 'backup'): (ProjectPermission, ProjectPermission.Scopes.EXPORT_BACKUP), - ('export', 'task', 'annotations'): (TaskPermission, TaskPermission.Scopes.EXPORT_ANNOTATIONS), - ('export', 'task', 'dataset'): (TaskPermission, TaskPermission.Scopes.EXPORT_DATASET), - ('export', 'task', 'backup'): (TaskPermission, TaskPermission.Scopes.EXPORT_BACKUP), - ('export', 'job', 'annotations'): (JobPermission, JobPermission.Scopes.EXPORT_ANNOTATIONS), - ('export', 'job', 'dataset'): (JobPermission, JobPermission.Scopes.EXPORT_DATASET), - }[(parsed_rq_id.action, parsed_rq_id.target, parsed_rq_id.subresource)] - - - resource = None - if (resource_id := parsed_rq_id.identifier) and isinstance(resource_id, int): - resource_model = { - 'project': Project, - 'task': Task, - 'job': Job, - }[parsed_rq_id.target] - - try: - resource = resource_model.objects.get(id=resource_id) - except resource_model.DoesNotExist as ex: - raise NotFound(f'The {parsed_rq_id.target!r} with specified id#{resource_id} does not exist') from ex - - permissions.append(permission_class.create_base_perm(request, view, scope=resource_scope, iam_context=iam_context, obj=resource)) - if scope != cls.Scopes.LIST: user_id = request.user.id if not is_rq_job_owner(obj, user_id):