fix(perms): restrict job deletion to superusers#1284
Conversation
The MLDataManager role granted delete_job, which ProjectManager inherited via permission union. Both roles could delete Job records through the API, but Job rows are kept in the DB for audit and traceability — only superusers should remove them. - Drop DELETE_JOB from MLDataManager.permissions (ProjectManager loses it via inherited union) - Add a data migration to scrub delete_job from existing per-project MLDataManager and ProjectManager guardian groups so the change takes effect on already-deployed environments - Add API tests covering MLDataManager / ProjectManager (403) and superuser (204) Roles that can run ML jobs retain the run_*_job permission, which the cancel endpoint already uses to terminate in-flight work without deleting the row. A follow-up to issue #1283 will generalize the cancel/cleanup helper and wire it into the destroy path so superuser deletions also tear down NATS/Redis state. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-ssec canceled.
|
✅ Deploy Preview for antenna-preview canceled.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis change revokes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 38 minutes and 23 seconds.Comment |
The frontend's ``canDelete`` getter (ui/src/data-services/models/job.ts) checks for ``"delete"`` in the serialized ``user_permissions`` array, and the jobs list only renders the hover trash icon when ``canDelete`` is true. Add detail-endpoint tests asserting the exact contents of that array for each role: - MLDataManager: no ``"delete"`` → no trash icon - ProjectManager: no ``"delete"`` → no trash icon - Superuser: ``"delete"`` present → trash icon renders This covers the surface the user actually interacts with, not just the DELETE endpoint return code. Co-Authored-By: Claude <noreply@anthropic.com>
|
Confirmed that a project manager or owner does not see the Trash icon for jobs in the UI and cannot delete Job records, only cancel them. 
|
There was a problem hiding this comment.
Pull request overview
Restricts destructive job deletion in the API by removing the delete_job permission from all project-scoped roles and revoking it from existing role groups in deployed environments, while keeping deletion available to superusers for audit/traceability needs.
Changes:
- Removed
delete_jobfrom theMLDataManagerrole (and therefore fromProjectManagervia inherited union). - Added a data migration to revoke
delete_jobfrom existing per-project Guardian role groups (MLDataManager,ProjectManager). - Added API tests ensuring role users receive
403on DELETE, while superusers can delete (204) and the row is removed; also verifiesuser_permissionsdoes/doesn’t include"delete"accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ami/users/roles.py | Stops granting delete_job via role permission sets. |
| ami/main/migrations/0084_revoke_delete_job_from_roles.py | Revokes delete_job from existing role groups immediately on deploy. |
| ami/jobs/tests/test_jobs.py | Adds coverage for DELETE authorization + user_permissions serialization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ami/main/migrations/0084_revoke_delete_job_from_roles.py (1)
22-67: ⚡ Quick winAdd a regression test for the migration path.
This protects the durable fix for deployed environments. The current API tests cover fresh role creation, but not a legacy group already holding
delete_jobbefore the migration runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/migrations/0084_revoke_delete_job_from_roles.py` around lines 22 - 67, Add a regression test that seeds a legacy Group that already has the delete_job Permission on the Project content type, runs the migration function remove_delete_job_from_role_groups (or applies the migration via Migration) and then asserts the group's permissions no longer include the delete_job Permission and any guardian GroupObjectPermission rows referencing that permission and the Project content type are removed; use apps.get_model to fetch auth.Group, auth.Permission, contenttypes.ContentType and guardian.GroupObjectPermission in the test, create the pre-migration state (assign permission both via group.permissions.add and a GroupObjectPermission row), invoke the migration, and assert both the permission removal from Group and deletion of GroupObjectPermission entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ami/main/migrations/0084_revoke_delete_job_from_roles.py`:
- Around line 22-67: Add a regression test that seeds a legacy Group that
already has the delete_job Permission on the Project content type, runs the
migration function remove_delete_job_from_role_groups (or applies the migration
via Migration) and then asserts the group's permissions no longer include the
delete_job Permission and any guardian GroupObjectPermission rows referencing
that permission and the Project content type are removed; use apps.get_model to
fetch auth.Group, auth.Permission, contenttypes.ContentType and
guardian.GroupObjectPermission in the test, create the pre-migration state
(assign permission both via group.permissions.add and a GroupObjectPermission
row), invoke the migration, and assert both the permission removal from Group
and deletion of GroupObjectPermission entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00df36c3-ed3c-4c6e-9cc7-de6aafc28dfc
📒 Files selected for processing (3)
ami/jobs/tests/test_jobs.pyami/main/migrations/0084_revoke_delete_job_from_roles.pyami/users/roles.py
The new TestJobDeletePermission class duplicated coverage already provided by TestRolePermissions in ami/main/tests.py, which asserts the user_permissions array on list responses (the same array the UI trash icon reads via Job.canDelete). Update the project_manager job permissions map to expect delete=False so existing coverage passes. Co-Authored-By: Claude <noreply@anthropic.com>
Summary
The
MLDataManagerrole granteddelete_jobfrom the initial roles framework (#693), andProjectManagerinherited it via the permission-set union. Both roles couldDELETE /api/v2/jobs/{id}/and remove Job rows.Job records are kept in the DB for audit / traceability — only superusers should delete them. Roles that need to stop in-flight work already have the
run_*_jobpermission, which the cancel endpoint uses to revoke and clean up without dropping the row.Changes
ami/users/roles.py— dropDELETE_JOBfromMLDataManager.permissions.ProjectManagerloses it via inherited union, no separate edit.ami/main/migrations/0084_revoke_delete_job_from_roles.py— data migration that stripsdelete_jobfrom existing per-projectMLDataManagerandProjectManagerguardian groups, so deployed environments lose the perm immediately rather than waiting for the nextpost_migrateto re-runcreate_roles_for_project.ami/jobs/tests/test_jobs.py— newTestJobDeletePermissioncovering:delete_jobMLDataManagerandProjectManagerusers get403on DELETE204and the row is goneThe
delete_jobpermission codename remains a valid permission onProject.Meta.permissions— it's just not granted to any role. Superusers bypass viais_superuser.Out of scope (followup)
Currently a superuser DELETE drops the Job row but does not revoke the Celery task or clean up NATS/Redis async state. That belongs to issue #1283 (cleanup-on-revoke / cleanup-before-restart). Posted an implementation proposal at #1283#issuecomment-4356728574 with a generic
Job.terminate()helper and the call sites that should funnel through it.Test plan
python manage.py test ami.jobs.tests.test_jobs.TestJobDeletePermission --keepdb— 5 tests pass locallypython manage.py makemigrations --check --dry-run— no missing migrationspython manage.py migrate main— 0084 applies cleanly on dev DBSummary by CodeRabbit
Bug Fixes
Tests