Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow quality reports to regular users #8511

Merged
merged 19 commits into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Changed

- \[Server API\] Quality report computation is now allowed to regular users
(<https://github.com/cvat-ai/cvat/pull/8511>)
1 change: 1 addition & 0 deletions cvat/apps/iam/rules/utils.rego
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ IMPORT_DATASET := "import:dataset"
IMPORT_BACKUP := "import:backup"
EXPORT_BACKUP := "export:backup"
UPDATE_ORG := "update:organization"
VIEW_STATUS := "view:status"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incomplete implementation of PR objectives

While the addition of the VIEW_STATUS constant is a step in the right direction, it appears that the implementation is incomplete based on the PR objectives. The PR aims to "allow regular users to compute quality reports for tasks," but the current changes only add a constant for viewing status.

To fully implement the desired functionality, consider the following:

  1. Add rules or functions that use the new VIEW_STATUS constant to grant permissions to regular users.
  2. Implement the actual computation of quality reports, which may involve changes in other files.
  3. Ensure that the permission check is integrated into the quality report computation process.

Would you like assistance in drafting the additional rules or functions needed to complete this implementation?



get_priority(privilege) := {
Expand Down
57 changes: 47 additions & 10 deletions cvat/apps/quality_control/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
class QualityReportPermission(OpenPolicyAgentPermission):
obj: Optional[QualityReport]
job_owner_id: Optional[int]
task_id: Optional[int]

class Scopes(StrEnum):
LIST = "list"
Expand All @@ -29,7 +30,7 @@
def create_scope_check_status(cls, request, job_owner_id: int, iam_context=None):
if not iam_context and request:
iam_context = get_iam_context(request, None)
return cls(**iam_context, scope="view:status", job_owner_id=job_owner_id)
return cls(**iam_context, scope=cls.Scopes.VIEW_STATUS, job_owner_id=job_owner_id)

@classmethod
def create_scope_view(cls, request, report: Union[int, QualityReport], iam_context=None):
Expand Down Expand Up @@ -59,11 +60,36 @@
elif scope == Scopes.LIST and isinstance(obj, Task):
permissions.append(TaskPermission.create_scope_view(request, task=obj))
elif scope == Scopes.CREATE:
if request.query_params.get("rq_id"):
# There will be another check for this case during request processing
continue

task_id = request.data.get("task_id")
if task_id is not None:
permissions.append(TaskPermission.create_scope_view(request, task_id))
# The request may have a different org or org unset
# Here we need to retrieve iam_context for this user, based on the task_id
try:
task = Task.objects.get(id=task_id)
except Task.DoesNotExist as ex:
raise ValidationError(str(ex))
Fixed Show fixed Hide fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid exposing sensitive information in exceptions

Raising ValidationError(str(ex)) may leak internal details to the user, which could be a security concern. It's better to provide a generic error message.

Apply this diff to fix the issue:

try:
    task = Task.objects.get(id=task_id)
except Task.DoesNotExist:
-    raise ValidationError(str(ex))
+    raise ValidationError(f"Task with id {task_id} does not exist.")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
task = Task.objects.get(id=task_id)
except Task.DoesNotExist as ex:
raise ValidationError(str(ex))
try:
task = Task.objects.get(id=task_id)
except Task.DoesNotExist:
raise ValidationError(f"Task with id {task_id} does not exist.")
🧰 Tools
🪛 GitHub Check: CodeQL

[warning] 74-74: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.

iam_context = get_iam_context(request, task)

permissions.append(
TaskPermission.create_scope_view(request, task, iam_context=iam_context)
)

permissions.append(
cls.create_base_perm(
request,
view,
scope,
iam_context,
obj,
task_id=task_id,
)
)

permissions.append(cls.create_base_perm(request, view, scope, iam_context, obj))
else:
permissions.append(cls.create_base_perm(request, view, scope, iam_context, obj))

Expand Down Expand Up @@ -91,15 +117,26 @@
def get_resource(self):
data = None

if self.obj:
task = self.obj.get_task()
if task.project:
if self.obj or self.scope == self.Scopes.CREATE:
task = None
if self.obj:
obj_id = self.obj.id
task = self.obj.get_task()
elif self.scope == self.Scopes.CREATE:
obj_id = None

if self.task_id:
task = Task.objects.get(id=self.task_id)
else:
raise AssertionError(self.scope)
zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved

if task and task.project:
organization = task.project.organization
else:
organization = task.organization
organization = getattr(task, "organization", None)
zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved

data = {
"id": self.obj.id,
"id": obj_id,
"organization": {"id": getattr(organization, "id", None)},
"task": (
{
Expand All @@ -114,12 +151,12 @@
"owner": {"id": getattr(task.project.owner, "id", None)},
"assignee": {"id": getattr(task.project.assignee, "id", None)},
}
if task.project
if getattr(task, "project", None)
else None
),
}
elif self.scope == self.Scopes.VIEW_STATUS:
data = {"owner": self.job_owner_id}
data = {"owner": {"id": self.job_owner_id}}

return data

Expand Down
67 changes: 46 additions & 21 deletions cvat/apps/quality_control/quality_reports.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,22 @@
from copy import deepcopy
from datetime import timedelta
from functools import cached_property, partial
from time import sleep
from typing import Any, Callable, Dict, Hashable, List, Optional, Sequence, Tuple, Union, cast
from uuid import uuid4

import datumaro as dm
import datumaro.util.mask_tools
import django_rq
import numpy as np
import rq
from attrs import asdict, define, fields_dict
from datumaro.util import dump_json, parse_json
from django.conf import settings
from django.db import transaction
from django.utils import timezone
from django_rq.queues import DjangoRQ as RqQueue
from rq.job import Job as RqJob
from rq_scheduler import Scheduler as RqScheduler
from scipy.optimize import linear_sum_assignment

from cvat.apps.dataset_manager.bindings import (
Expand All @@ -48,6 +52,7 @@
User,
ValidationMode,
)
from cvat.apps.engine.utils import define_dependent_job, get_rq_job_meta, get_rq_lock_by_user
from cvat.apps.profiler import silk_profile
from cvat.apps.quality_control import models
from cvat.apps.quality_control.models import (
Expand Down Expand Up @@ -2115,25 +2120,26 @@ def generate_report(self) -> ComparisonReport:


class QualityReportUpdateManager:
_QUEUE_JOB_PREFIX = "update-quality-metrics-task-"
_QUEUE_AUTOUPDATE_JOB_PREFIX = "update-quality-metrics-"
_QUEUE_CUSTOM_JOB_PREFIX = "quality-check-"
_RQ_CUSTOM_QUALITY_CHECK_JOB_TYPE = "custom_quality_check"
_JOB_RESULT_TTL = 120

@classmethod
def _get_quality_check_job_delay(cls) -> timedelta:
return timedelta(seconds=settings.QUALITY_CHECK_JOB_DELAY)

def _get_scheduler(self):
def _get_scheduler(self) -> RqScheduler:
return django_rq.get_scheduler(settings.CVAT_QUEUES.QUALITY_REPORTS.value)

def _get_queue(self):
def _get_queue(self) -> RqQueue:
return django_rq.get_queue(settings.CVAT_QUEUES.QUALITY_REPORTS.value)

def _make_queue_job_id_base(self, task: Task) -> str:
return f"{self._QUEUE_JOB_PREFIX}{task.id}"
return f"{self._QUEUE_AUTOUPDATE_JOB_PREFIX}task-{task.id}"

def _make_custom_quality_check_job_id(self) -> str:
return uuid4().hex
def _make_custom_quality_check_job_id(self, task_id: int, user_id: int) -> str:
return f"{self._QUEUE_CUSTOM_JOB_PREFIX}task-{task_id}-user-{user_id}"

@classmethod
def _get_last_report_time(cls, task: Task) -> Optional[timezone.datetime]:
Expand Down Expand Up @@ -2186,28 +2192,47 @@ def schedule_quality_autoupdate_job(self, task: Task):
task_id=task.id,
)

def schedule_quality_check_job(self, task: Task, *, user_id: int) -> str:
class JobAlreadyExists(QualityReportsNotAvailable):
def __str__(self):
return "Quality computation job for this task already enqueued"
Comment on lines +2199 to +2201
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Adjust inheritance of JobAlreadyExists exception

The JobAlreadyExists exception currently inherits from QualityReportsNotAvailable, which might not be semantically appropriate. Since JobAlreadyExists represents a different kind of error (a job is already enqueued), it would be clearer to inherit from a more general exception class like Exception or define a new base exception class for job scheduling errors.

Apply this diff to adjust the inheritance:

-class JobAlreadyExists(QualityReportsNotAvailable):
+class JobAlreadyExists(Exception):
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class JobAlreadyExists(QualityReportsNotAvailable):
def __str__(self):
return "Quality computation job for this task already enqueued"
class JobAlreadyExists(Exception):
def __str__(self):
return "Quality computation job for this task already enqueued"


def schedule_custom_quality_check_job(self, request, task: Task, *, user_id: int) -> str:
"""
Schedules a quality report computation job, supposed for updates by a request.
"""

self._check_quality_reporting_available(task)

rq_id = self._make_custom_quality_check_job_id()

queue = self._get_queue()
queue.enqueue(
self._check_task_quality,
task_id=task.id,
job_id=rq_id,
meta={"user_id": user_id, "job_type": self._RQ_CUSTOM_QUALITY_CHECK_JOB_TYPE},
result_ttl=self._JOB_RESULT_TTL,
failure_ttl=self._JOB_RESULT_TTL,
)

with get_rq_lock_by_user(queue, user_id=user_id):
rq_id = self._make_custom_quality_check_job_id(task_id=task.id, user_id=user_id)
rq_job = queue.fetch_job(rq_id)
if rq_job and rq_job.get_status(refresh=False) in (
rq.job.JobStatus.QUEUED,
rq.job.JobStatus.STARTED,
rq.job.JobStatus.SCHEDULED,
):
raise self.JobAlreadyExists()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential exceptions when fetching job status

Fetching the job status may raise exceptions if the job does not exist or if there's a connection issue with the queue. It's important to handle these exceptions to prevent crashes and provide meaningful error messages.

Consider adding exception handling around the job fetching and status checking:

try:
    rq_job = queue.fetch_job(rq_id)
    if rq_job and rq_job.get_status(refresh=False) in (
        rq.job.JobStatus.QUEUED,
        rq.job.JobStatus.STARTED,
        rq.job.JobStatus.SCHEDULED,
    ):
        raise self.JobAlreadyExists()
except Exception as e:
    logger.error(f"Failed to fetch job status for {rq_id}: {e}")
    # Handle the exception as appropriate


dependency = define_dependent_job(
queue, user_id=user_id, rq_id=rq_id, should_be_dependent=True
)

queue.enqueue(
self._check_task_quality,
task_id=task.id,
job_id=rq_id,
meta=get_rq_job_meta(request=request, db_obj=task),
result_ttl=self._JOB_RESULT_TTL,
failure_ttl=self._JOB_RESULT_TTL,
depends_on=dependency,
)
print(queue.fetch_job(rq_id).get_status())

return rq_id

def get_quality_check_job(self, rq_id: str):
def get_quality_check_job(self, rq_id: str) -> Optional[RqJob]:
queue = self._get_queue()
rq_job = queue.fetch_job(rq_id)

Expand All @@ -2216,8 +2241,8 @@ def get_quality_check_job(self, rq_id: str):

return rq_job

def is_custom_quality_check_job(self, rq_job) -> bool:
return rq_job.meta.get("job_type") == self._RQ_CUSTOM_QUALITY_CHECK_JOB_TYPE
def is_custom_quality_check_job(self, rq_job: RqJob) -> bool:
return isinstance(rq_job.id, str) and rq_job.id.startswith(self._QUEUE_CUSTOM_JOB_PREFIX)
zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved

@classmethod
@silk_profile()
Expand Down
59 changes: 59 additions & 0 deletions cvat/apps/quality_control/rules/conflicts.rego
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import rego.v1

import data.utils
import data.organizations
import data.tasks

# input: {
# "scope": <"list"> or null,
Expand Down Expand Up @@ -41,6 +42,42 @@ import data.organizations
# }
# }

is_task_owner if {
input.resource.task.owner.id == input.auth.user.id
}

is_task_assignee if {
input.resource.task.assignee.id == input.auth.user.id
}
zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved

is_project_owner if {
input.resource.project.owner.id == input.auth.user.id
}

is_project_assignee if {
input.resource.project.assignee.id == input.auth.user.id
}
zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved

is_project_staff if {
is_project_owner
}

is_project_staff if {
is_project_assignee
}

zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved
is_task_staff if {
is_project_staff
}

is_task_staff if {
is_task_owner
}

is_task_staff if {
is_task_assignee
}

zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved
default allow := false

allow if {
Expand All @@ -57,6 +94,28 @@ allow if {
organizations.is_member
}

allow if {
input.scope == utils.VIEW
utils.is_sandbox
is_task_staff
utils.has_perm(utils.WORKER)
}

allow if {
input.scope == utils.VIEW
input.auth.organization.id == input.resource.organization.id
utils.has_perm(utils.USER)
organizations.has_perm(organizations.MAINTAINER)
}

zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved
allow if {
input.scope == utils.VIEW
is_task_staff
input.auth.organization.id == input.resource.organization.id
utils.has_perm(utils.WORKER)
organizations.has_perm(organizations.WORKER)
}

zhiltsov-max marked this conversation as resolved.
Show resolved Hide resolved
filter := [] if { # Django Q object to filter list of entries
utils.is_admin
utils.is_sandbox
Expand Down
Loading
Loading