-
Notifications
You must be signed in to change notification settings - Fork 5
Permissions based on job type #891
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
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
@mohamedelabbas1996 Will you share a preview of the updated API response with all the new permissions? |
…/RolnickLab/antenna into feat/restrict-ml-processing-jobs
"""Custom permission to check if the user can populate a collection.""" | ||
|
||
permission = Project.Permissions.POPULATE_COLLECTION | ||
return True # Always allow — object-level handles actual checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to default this to False?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here if has permission returns False it will just deny access and won't even check object level permissions.
This is because has_permission() is evaluated first for view-level access, and only if it passes does DRF proceed to check object-level permissions (if applicable). So even if has_object_permission() would allow the action, it won’t matter if has_permission() blocks the request upfront.
728a05d
to
dd35803
Compare
@mohamedelabbas1996 Great work here. I brought this PR up-to-date with main and recreated the migrations. I am doing some testing and have run into a couple things:
![]() ![]()
![]()
![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors job permissions to support fine-grained control based on job types, allowing administrators to disable ML processing jobs on collections while still permitting individual image processing and other job types.
Key changes include:
- Replaced generic job permissions (
run_job
,cancel_job
,retry_job
) with type-specific permissions (run_ml_job
,run_populate_captures_collection_job
, etc.) - Added
run_single_image_ml_job
permission for processing individual captures - Consolidated permission framework by moving logic from dedicated permission classes to model methods
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
ui/src/utils/user/types.ts |
Updated enum to remove deprecated permissions and add single image processing permission |
ui/src/pages/session-details/playback/capture-details/capture-job/process-now.tsx |
Updated to use new single image permission for ML job processing |
ui/src/data-services/models/job.ts |
Changed canCancel and canRetry to use generic Run permission instead of specific Cancel/Retry permissions |
ui/src/data-services/models/capture-details.ts |
Added userPermissions getter to expose permissions for capture processing |
ami/users/roles.py |
Updated role definitions to use new fine-grained job permissions and grant single image processing to basic members |
ami/main/models.py |
Added new permission constants and implemented unified permission checking framework in BaseModel |
ami/jobs/models.py |
Implemented custom permission logic for job-specific actions based on job type |
ami/base/models.py |
Added comprehensive permission checking infrastructure to BaseModel |
ami/base/permissions.py |
Simplified to single ObjectPermission class that delegates to model methods |
ami/main/api/views.py |
Replaced specific permission classes with unified ObjectPermission |
ami/jobs/views.py |
Updated job creation to check run permissions when start_now parameter is used |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
"run_single_image": True, | ||
"run": False, | ||
"retry": False, | ||
"cancel": False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test data structure includes deprecated permission keys "run", "retry", "cancel" which should be removed since these permissions no longer exist according to the PR description. Only keep the new fine-grained permissions.
"cancel": False, |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohamedelabbas1996 is this true? should the generic run / retry / cancel be removed from the tests?
|
||
permissions = Permission.objects.filter(codename__in=deprecated_codenames) | ||
for perm in permissions: | ||
print(f"Deleting permission: {perm.codename}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using print()
statements in Django migrations is not recommended. Use Django's logging framework or remove the print statement entirely since the migration should run silently.
print(f"Deleting permission: {perm.codename}") |
Copilot uses AI. Check for mistakes.
class Migration(migrations.Migration): | ||
dependencies = [ | ||
("main", "0059_alter_project_options"), | ||
("main", "0058_alter_project_options"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration dependency appears incorrect. This migration references "0058_alter_project_options" but should likely depend on a more recent migration (possibly "0067_delete_deprecated_permissions") based on the migration sequence shown in other files.
("main", "0058_alter_project_options"), | |
("main", "0067_delete_deprecated_permissions"), |
Copilot uses AI. Check for mistakes.
Thanks for catching this, Michael. The problem isn’t with the run_single_image_ml_job check itself, but with what happens when the Process Now button is clicked. The button triggers two actions:
Since Basic Members don’t have the create_job permission by default, the request fails even though they do have run_single_image_ml_job. I see two possible solutions here: Option A: Grant Basic Members the create_job permission so they can create single-image jobs. Option B: Make a distinction in the JobView for the “run single image job” action so the view recognizes it directly as a single-image run, without passing through the generic create_job action and its permission check. |
Thanks @mohamedelabbas1996, I'm happy the permission system is working as designed! I agree we can make a couple changes though to make it more clear.
Will you take care of item Also... will you add basic instructions of how to add permissions to roles in the Django admin? Or link to those if you already wrote them? Projects -> Project Detail -> Object Permissions -> Search group name. Mention where NOT to try to change things (existing model permissions). And add a note that we will add an API to make that more streamlined. Thank you!! |
Thanks Michael! I’ve granted Basic Members the create_job permission so they can now create and run single-image jobs. I also linked the Roles & Permissions wiki guide in the How to Test the Changes section of the PR. Finally, I created a ticket for a dedicated action for run single image job. |
Fantastic @mohamedelabbas1996 I tested:
Success! |
Project owner/manager cannot run batch ml jobs. Only superusers can. I think you are using a superuser account to create the project here. |
Summary
This PR introduces fine-grained permission control for running jobs, specifically to support disabling ML processing jobs on source image collections while still allowing individual image processing.
It also introduces a refactor to the existing permissions framework: permission checks are moved from individual permission classes per model to a unified method in the base model. This method can be inherited and overridden by all child models. A single generic permission check class is now used across views, delegating the object-level permission logic to the model itself.
List of Changes
run_job
,cancel_job
, andretry_job
permissions on theJob
model.run_<jobtype>_job
permissions for each job type. These fine-grained permissions are now used to control access torun
,retry
, andcancel
actions per job type.create_job
,update_job
,delete_job
) remain shared across all job types.run_single_image_ml_job
permission added to support running ML jobs on individual source images; granted to all project members by default.create_job
permission.run
,cancel
andretry
permissions.Related Issues
Detailed Description
This change enables more flexible control over which users can run which job types. Instead of treating job run permissions as global, we now assign them based on the job type. This is useful for temporarily disabling ML processing jobs across the platform while still allowing users to run other types of jobs (e.g., export or sync jobs).
Additionally, a new
run_single_image_ml_job
permission allows all project members to run ML jobs on single source images even ifrun_ml_job
is disabled for them.All permission checks have been moved from dedicated permission classes to model methods, improving flexibility and maintainability.
How to Test the Changes
Note: we plan to add an API and a frontend page for managing project roles and permissions in a future iteration to streamline this workflow. UI For managing project membership & roles #727
run_<job_type>_job
permission can only run the permitted job types.user_permissions
values in the job and capture detail APIs.run_ml_job
permission cannot run ML jobs on collections.run_single_image_ml_job
permission enables running ML jobs on individual images.Deployment Notes
Checklist