Skip to content

Conversation

@mohamedelabbas1996
Copy link
Contributor

@mohamedelabbas1996 mohamedelabbas1996 commented Dec 18, 2025

Summary

This PR fixes an issue where only superusers could create data exports, preventing project managers and researchers from performing this action. It applies object-level permissions to the exports viewset and replaces the single TRIGGER_EXPORT permission with separate create, update, and delete permissions for finer access control.

List of Changes

TBD

Related Issues

#1078

Detailed Description

TBD

How to Test the Changes

TBD

Screenshots

TBD

Deployment Notes

TBD

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.

Summary by CodeRabbit

Release Notes

  • Permissions & Access Control
    • Permission enforcement now applied to export management operations.
    • Data export permissions restructured to enable granular control over create, update, and delete operations.
    • Researcher role permissions updated to reflect new export permission structure.

✏️ Tip: You can customize this high-level summary in your review settings.

@netlify
Copy link

netlify bot commented Dec 18, 2025

Deploy Preview for antenna-preview ready!

Name Link
🔨 Latest commit 19e6a47
🔍 Latest deploy log https://app.netlify.com/projects/antenna-preview/deploys/69443c159f2f340008d40d9e
😎 Deploy Preview https://deploy-preview-1077--antenna-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 35 (🔴 down 3 from production)
Accessibility: 89 (🟢 up 9 from production)
Best Practices: 92 (🔴 down 8 from production)
SEO: 100 (🟢 up 8 from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link

netlify bot commented Dec 18, 2025

Deploy Preview for antenna-ssec ready!

Name Link
🔨 Latest commit 19e6a47
🔍 Latest deploy log https://app.netlify.com/projects/antenna-ssec/deploys/69443c15a983df0007d68d6b
😎 Deploy Preview https://deploy-preview-1077--antenna-ssec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 18, 2025

Walkthrough

This change refactors data export permissions from a single TRIGGER_EXPORT permission to three granular permissions (CREATE, UPDATE, DELETE). The ExportViewSet is updated to enforce ObjectPermission checks on unsaved export objects before persisting them.

Changes

Cohort / File(s) Summary
Permission model refactoring
ami/main/models.py, ami/users/roles.py
Replaced TRIGGER_EXPORT with three new permission constants: CREATE_DATA_EXPORT, UPDATE_DATA_EXPORT, DELETE_DATA_EXPORT. Updated Researcher role to use new granular permissions.
Migration
ami/main/migrations/0079_alter_project_options.py
Generated Django 4.2.10 migration updating Project Meta options with new ordering ["-priority", "created_at"] and 34+ permission tuples covering CRUD operations across export, identification, deployment, and related entities.
ViewSet permission enforcement
ami/exports/views.py
Added ObjectPermission import and permission_classes assignment to ExportViewSet. Modified creation flow to instantiate unsaved DataExport object, perform permission check, then save and enqueue job.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Migration permissions verification: Audit all 34+ permission tuples in 0079_alter_project_options.py to ensure correctness and completeness
  • Permission enforcement logic: Verify that the unsaved object permission check in ExportViewSet correctly validates access before database persistence
  • Permission constant consistency: Confirm CREATE/UPDATE/DELETE_DATA_EXPORT mappings across models, migrations, and roles are aligned
  • Completeness check: Ensure no other roles or views require updates to the new permission scheme

Poem

🐰 Permissions fine-grained now appear,
Where TRIGGER once ruled, three friends are here!
CREATE, UPDATE, DELETE so clean,
The export control system's now pristine! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ⚠️ Warning The PR description is incomplete with multiple 'TBD' placeholders for critical sections including List of Changes, Detailed Description, testing instructions, and deployment notes. Complete all 'TBD' sections: provide a detailed list of changed files, explain the permission system changes, document testing procedures, and specify any deployment requirements.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title directly references the primary change: fixing exports permissions by applying object-level permissions and refining permission controls.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/exports-permissions

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mohamedelabbas1996 mohamedelabbas1996 self-assigned this Dec 18, 2025
@mohamedelabbas1996 mohamedelabbas1996 marked this pull request as draft December 18, 2025 17:45
@mihow
Copy link
Collaborator

mihow commented Dec 18, 2025

Thanks for fixing this one @mohamedelabbas1996. It's interesting that we didn't already have a permission for exports and no one has complained. Did something change?

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.

Project managers and researchers cannot create data exports

3 participants