Skip to content

Conversation

mohamedelabbas1996
Copy link
Contributor

@mohamedelabbas1996 mohamedelabbas1996 commented Jul 2, 2025

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

  • Removed the generic run_job, cancel_job, and retry_job permissions on the Job model.
  • Introduced fine-grained run_<jobtype>_job permissions for each job type. These fine-grained permissions are now used to control access to run, retry, and cancel actions per job type.
  • CRUD permissions (create_job, update_job, delete_job) remain shared across all job types.
  • A new run_single_image_ml_job permission added to support running ML jobs on individual source images; granted to all project members by default.
  • Only superusers can run batch ml jobs.
  • Frontend updated to reflect new permissions.
  • Permission framework refactored to move permission checks into the model instead of using permission check classes.
  • Granted project members create_job permission.
  • A data migrations is added to clean up old generic job run, cancel and retry 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 if run_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

  • See the assigning-object-level-permissions-via-django-admin for details on how to assign roles and permissions from django admin.
    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
  • Verify that users with run_<job_type>_job permission can only run the permitted job types.
  • Confirm that front-end has appropriate user_permissions values in the job and capture detail APIs.
  • Check that users without run_ml_job permission cannot run ML jobs on collections.
  • Validate that run_single_image_ml_job permission enables running ML jobs on individual images.

Deployment Notes

  • Executing the data migration should clean up the deprecated generic job permissions.

Checklist

  • I have tested these changes appropriately.
  • I have added and/or modified relevant tests.
  • I updated relevant documentation or comments.
  • I have verified that this PR follows the project's coding standards.
  • Any dependent changes have already been merged to main.

Copy link

netlify bot commented Jul 2, 2025

Deploy Preview for antenna-preview canceled.

Name Link
🔨 Latest commit 1fd3f5d
🔍 Latest deploy log https://app.netlify.com/projects/antenna-preview/deploys/68a383e059b92a0009ea7070

@mohamedelabbas1996 mohamedelabbas1996 linked an issue Jul 2, 2025 that may be closed by this pull request
@mihow
Copy link
Collaborator

mihow commented Jul 2, 2025

@mohamedelabbas1996 Will you share a preview of the updated API response with all the new permissions?

@mihow mihow added this to the Self-service improvements v1 milestone Jul 16, 2025
"""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
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

@mihow mihow force-pushed the feat/restrict-ml-processing-jobs branch from 728a05d to dd35803 Compare August 15, 2025 01:13
@mihow
Copy link
Collaborator

mihow commented Aug 15, 2025

@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:

  • I cannot process a single source image if I am only a Basic Project Member, even though that group has the run_single_image_ml_job permission
image image
  • The Process Now button in the UI is properly enabled or disabled based on the run_single_image_ml_job object permission on the BasicMember group! But the action fails because of the issue above.

  • The permission for the run ml jobs is named differently than other ones (run/retry/cancel) should they all be the same?

image
  • If I add all job permissions to the BasicMember role, I still am not able to see the buttons in the UI. Is the UI checking role names or permissions?
image

@mihow mihow requested a review from Copilot August 15, 2025 02:03
Copy link
Contributor

@Copilot Copilot AI left a 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,
Copy link
Preview

Copilot AI Aug 15, 2025

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.

Suggested change
"cancel": False,

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

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}")
Copy link
Preview

Copilot AI Aug 15, 2025

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.

Suggested change
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"),
Copy link
Preview

Copilot AI Aug 15, 2025

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.

Suggested change
("main", "0058_alter_project_options"),
("main", "0067_delete_deprecated_permissions"),

Copilot uses AI. Check for mistakes.

@mohamedelabbas1996
Copy link
Contributor Author

mohamedelabbas1996 commented Aug 18, 2025

  • I cannot process a single source image if I am only a Basic Project Member, even though that group has the run_single_image_ml_job permission

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:

  • create_job (to create the job object)

  • run_single_image_ml_job (to actually run it on the image)

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.

@mohamedelabbas1996
Copy link
Contributor Author

mohamedelabbas1996 commented Aug 18, 2025

  • If I add all job permissions to the BasicMember role, I still am not able to see the buttons in the UI. Is the UI checking role names or permissions?

For this one, make sure to add all the job permissions , including the CRUD permissions (create_job, update_job, delete_job) . I noticed only the run permissions were added in your test.

The UI doesn’t check role names; it relies on the permission names returned in the user_permissions list field in the API response.
If I add only the create_job permission, you can only see the create button:
image
image
image
If I add the run permissions, only start/retry/cancel buttons are shown
image
image
image

If add all other job permissions, all buttons show properly:
image
image

@mohamedelabbas1996
Copy link
Contributor Author

mohamedelabbas1996 commented Aug 18, 2025

  • The permission for the run ml jobs is named differently than other ones (run/retry/cancel) should they all be the same?

If you apply the migration 0068_alter_project_options, the naming should be consistent across all job types

("run_ml_job", "Can run/retry/cancel ML jobs"),

image

I think it makes sense for all run permissions to follow this pattern, since the run_* permissions are used not only to start jobs but also to retry and cancel them.

@mihow
Copy link
Collaborator

mihow commented Aug 18, 2025

  • I cannot process a single source image if I am only a Basic Project Member, even though that group has the run_single_image_ml_job permission

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:

  • create_job (to create the job object)
  • run_single_image_ml_job (to actually run it on the image)

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.

  1. Add create_job permissions by default to basic members. That makes sense to me. They can create job entires, but only run some types (which is where the work is actually performed). And there may be some basic background jobs that even basic members will need to trigger (e.g. update stats). I don't think we can add modify or delete permissions. The effect of modifying & deleting jobs is unknown. I think only super users can do that now.

  2. I like the idea of updating the view to skip the create_job action. I consolidated the code for processing single images into it's own function here https://github.com/RolnickLab/antenna/pull/909/files#diff-6b8cf02ffac202cfc91f1bcc7be867564a10038472ae78939472cea5940ba400R11
    however I think this changes is optional for this PR.

Will you take care of item #1? and then add a new ticket for#2?

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!!

@mohamedelabbas1996
Copy link
Contributor Author

  • I cannot process a single source image if I am only a Basic Project Member, even though that group has the run_single_image_ml_job permission

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:

  • create_job (to create the job object)
  • run_single_image_ml_job (to actually run it on the image)

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.

1. Add `create_job` permissions by default to basic members. That makes sense to me. They can create job entires, but only `run` some types (which is where the work is actually performed). And there may be some basic background jobs that even basic members will need to trigger (e.g. update stats). I don't think we can add modify or delete permissions. The effect of modifying & deleting jobs is unknown. I think only super users can do that now.

2. I like the idea of updating the view to skip the `create_job` action. I consolidated the code for processing single images into it's own function here https://github.com/RolnickLab/antenna/pull/909/files#diff-6b8cf02ffac202cfc91f1bcc7be867564a10038472ae78939472cea5940ba400R11
   however I think this changes is optional for this PR.

Will you take care of item #1? and then add a new ticket for#2?

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.

@mihow
Copy link
Collaborator

mihow commented Aug 20, 2025

Fantastic @mohamedelabbas1996

I tested:

  • Creating a new project
  • Project owner can create and run single image jobs, and batch ml jobs
  • Added new member
  • Basic project member can create and start a single image job with the "Process Now" button
  • Basic project member can create jobs, but not start them
  • I added the run ml job permission in the Object Permissions view directly to the user
  • The basic project member can now run batch ML jobs.

Success!

@mihow mihow merged commit 28571b0 into main Aug 20, 2025
10 checks passed
@mihow mihow deleted the feat/restrict-ml-processing-jobs branch August 20, 2025 00:52
@mohamedelabbas1996
Copy link
Contributor Author

mohamedelabbas1996 commented Aug 20, 2025

  • Project owner can create and run single image jobs, and batch ml jobs

Project owner/manager cannot run batch ml jobs. Only superusers can. I think you are using a superuser account to create the project here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable ability to start ML jobs for non-admins
3 participants