Skip to content

Commit

Permalink
Don't check permissions for the target resource when canceling reques…
Browse files Browse the repository at this point in the history
…ts (#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.
  • Loading branch information
SpecLad authored Aug 30, 2024
1 parent 61a7f34 commit 2d6ac62
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 37 deletions.
5 changes: 5 additions & 0 deletions changelog.d/20240828_053041_roman_rm_extra_checks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### Changed

- When cancelling a request, a user is no longer required to have
permissions to perform the original action
(<https://github.com/cvat-ai/cvat/pull/8369>)
38 changes: 1 addition & 37 deletions cvat/apps/engine/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down

0 comments on commit 2d6ac62

Please sign in to comment.