Skip to content

Allow quality reports to regular users #8511

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

Merged
merged 19 commits into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from 7 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))

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)

if task and task.project:
organization = task.project.organization
else:
organization = task.organization
organization = getattr(task, "organization", None)

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
66 changes: 45 additions & 21 deletions cvat/apps/quality_control/quality_reports.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,20 @@
from datetime import timedelta
from functools import cached_property, partial
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 +51,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 +2119,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 +2191,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 +2240,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)

@classmethod
@silk_profile()
Expand Down
23 changes: 23 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.quality_utils

# input: {
# "scope": <"list"> or null,
Expand Down Expand Up @@ -57,6 +58,28 @@ allow if {
organizations.is_member
}

allow if {
input.scope == utils.VIEW
utils.is_sandbox
quality_utils.is_task_staff(input.resource.task, input.resource.project, input.auth)
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)
}

allow if {
input.scope == utils.VIEW
quality_utils.is_task_staff(input.resource.task, input.resource.project, input.auth)
input.auth.organization.id == input.resource.organization.id
utils.has_perm(utils.WORKER)
organizations.has_perm(organizations.WORKER)
}

filter := [] if { # Django Q object to filter list of entries
utils.is_admin
utils.is_sandbox
Expand Down
32 changes: 31 additions & 1 deletion cvat/apps/quality_control/rules/quality_reports.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.quality_utils

# input: {
# "scope": <"view"|"list"|"create"|"view:status"> or null,
Expand All @@ -23,7 +24,7 @@ import data.organizations
# } or null,
# },
# "resource": {
# "id": <num>,
# "id": <num> or null,
# "owner": { "id": <num> },
# "organization": { "id": <num> } or null,
# "task": {
Expand All @@ -41,6 +42,8 @@ import data.organizations
# }
# }



default allow := false

allow if {
Expand All @@ -57,6 +60,33 @@ allow if {
organizations.is_member
}

allow if {
input.scope == utils.VIEW_STATUS
input.auth.user.id == input.resource.owner.id
}

allow if {
input.scope in {utils.CREATE, utils.VIEW}
utils.is_sandbox
quality_utils.is_task_staff(input.resource.task, input.resource.project, input.auth)
utils.has_perm(utils.WORKER)
}

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

allow if {
input.scope in {utils.CREATE, utils.VIEW}
quality_utils.is_task_staff(input.resource.task, input.resource.project, input.auth)
input.auth.organization.id == input.resource.organization.id
utils.has_perm(utils.WORKER)
organizations.has_perm(organizations.WORKER)
}

filter := [] if { # Django Q object to filter list of entries
utils.is_admin
utils.is_sandbox
Expand Down
40 changes: 40 additions & 0 deletions cvat/apps/quality_control/rules/quality_utils.rego
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package quality_utils

import rego.v1


is_task_owner(task_data, auth_data) if {
task_data.owner.id == auth_data.user.id
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to move to engine at some point


is_task_assignee(task_data, auth_data) if {
task_data.assignee.id == auth_data.user.id
}

is_project_owner(project_data, auth_data) if {
project_data.owner.id == auth_data.user.id
}

is_project_assignee(project_data, auth_data) if {
project_data.assignee.id == auth_data.user.id
}

is_project_staff(project_data, auth_data) if {
is_project_owner(project_data, auth_data)
}

is_project_staff(project_data, auth_data) if {
is_project_assignee(project_data, auth_data)
}

is_task_staff(task_data, project_data, auth_data) if {
is_project_staff(project_data, auth_data)
}

is_task_staff(task_data, project_data, auth_data) if {
is_task_owner(task_data, auth_data)
}

is_task_staff(task_data, project_data, auth_data) if {
is_task_assignee(task_data, auth_data)
}
2 changes: 1 addition & 1 deletion cvat/apps/quality_control/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class Meta:


class QualityReportCreateSerializer(serializers.Serializer):
task_id = serializers.IntegerField(write_only=True)
task_id = serializers.IntegerField(write_only=True, required=False)


class QualitySettingsSerializer(serializers.ModelSerializer):
Expand Down
Loading
Loading