Skip to content

Commit

Permalink
Don't check permissions for the underlying resource when canceling re…
Browse files Browse the repository at this point in the history
…quests

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 committed Aug 28, 2024
1 parent ec28076 commit 6de27fd
Showing 1 changed file with 0 additions and 36 deletions.
36 changes: 0 additions & 36 deletions cvat/apps/engine/permissions.py
Original file line number Diff line number Diff line change
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 6de27fd

Please sign in to comment.