Extend permissions framework to support model-level permissions #1010
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
mohamedelabbas1996
changed the title
ami/main/models.py
Outdated
| class Meta: | ||
| ordering = ["-priority", "created_at"] | ||
| permissions = [ | ||
| # Project permissions |
There was a problem hiding this comment.
Can this be moved to base.models.BaseModel.Meta?
You have to explicitly inherit from the class Meta(BaseModel.Meta)
There was a problem hiding this comment.
Adding create_project here makes sense for the Project model! but for other models, they could either go on that model or the BaseModel with a dynamic method.
There was a problem hiding this comment.
I tried moving permissions to the BaseModel, but it turns out Django creates duplicate permissions when they’re inherited one for the BaseModel itself and another for each subclass like Project.
We could instead handle this through a signal that automatically creates model-level permissions for each relevant model at startup.
There was a problem hiding this comment.
I don't mind having all the permissions listed under Project.Meta because I think we will move them to Organization.Meta later. Otherwise, I would say put them each under a relevant model like ProcessingService.Meta
… ProcessingService models
There was a problem hiding this comment.
Pull Request Overview
This PR extends the Antenna permissions framework to support both object-level and model-level permissions, addressing the limitation where only object-level permissions were previously checked using the related project. The enhancement introduces a hybrid system that falls back to Django's model-level permissions when a model is not directly tied to a specific project, and enables all authenticated users to create projects by introducing a new AuthenticatedUsers global role.
Key changes:
- Added model-level permission checking logic to handle models with many-to-many project relationships or no project relation
- Introduced
GlobalRolebase class andAuthenticatedUsersrole to grantcreate_projectpermission to all authenticated users - Added automatic role assignment via post-save signal for new users
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ami/users/signals.py | Added post_save signal to auto-assign AuthenticatedUsers role to newly created users |
| ami/users/roles.py | Renamed permissions to object_level_permissions, introduced GlobalRole base class and AuthenticatedUsers role |
| ami/ml/views.py | Added ObjectPermission class and permission check before creating ProcessingService instances |
| ami/ml/tests.py | Updated test user to be superuser to maintain test validity |
| ami/main/tests.py | Added comprehensive test coverage for model-level permissions on Project, ProcessingService, and Taxon models |
| ami/main/models.py | Added permission check methods to Project model and renamed custom permission methods for clarity |
| ami/main/migrations/0080_alter_project_options.py | Added model-level permissions to Project model metadata |
| ami/main/migrations/0079_assign_authenticated_users_group.py | Migration to create AuthenticatedUsers group and assign it to all existing users |
| ami/main/api/views.py | Added ObjectPermission class to TaxonViewSet |
| ami/jobs/views.py | Renamed check_custom_permission to check_custom_object_level_permission |
| ami/jobs/models.py | Renamed custom permission methods and updated logging message |
| ami/base/serializers.py | Added get_instance_for_permission_check method to build unsaved instances for permission validation |
| ami/base/permissions.py | Refactored to use new get_permissions method and model's get_collection_level_permissions |
| ami/base/models.py | Added comprehensive model-level and object-level permission infrastructure with proper fallback logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def assign_model_level_permissions(cls, group): | ||
| from django.contrib.contenttypes.models import ContentType | ||
|
|
||
| ct = ContentType.objects.get_for_model(Project) |
There was a problem hiding this comment.
The assign_model_level_permissions method hardcodes the content type to Project, which limits the GlobalRole class to only Project permissions. Consider making this more generic by accepting a model class parameter or deriving the content type from the permission codename.
| logger.debug(f"Project.check_permission action: {action}") | ||
| if action == "create": | ||
| return self.check_model_level_permission(user, action) | ||
| return super().check_object_level_permission(user, action) |
There was a problem hiding this comment.
The method calls super().check_object_level_permission(user, action) on line 329, but check_permission is the entry point that should be called. This bypasses the logic in the parent's check_permission method. Call super().check_permission(user, action) instead to ensure proper permission routing.
| return super().check_object_level_permission(user, action) | |
| return super().check_permission(user, action) |
| cls, user: AbstractUser | AnonymousUser, project: "Project | None" = None | ||
| ) -> list[str]: | ||
| # Use model-level permissions for project collection-level actions | ||
| return ["create"] if user.has_perm("main.create_project") else [] |
There was a problem hiding this comment.
The parameter order differs from the parent class method in ami/base/models.py line 326, which has project before user. This inconsistency could lead to errors when calling the method. Align the parameter order with the base class.
| cls, user: AbstractUser | AnonymousUser, project: "Project | None" = None | |
| ) -> list[str]: | |
| # Use model-level permissions for project collection-level actions | |
| return ["create"] if user.has_perm("main.create_project") else [] | |
| cls, project: "Project | None" = None, user: AbstractUser | AnonymousUser = None | |
| ) -> list[str]: | |
| # Use model-level permissions for project collection-level actions | |
| return ["create"] if user and user.has_perm("main.create_project") else [] |
ami/base/models.py
Outdated
|
|
||
| def check_model_level_permission(self, user: AbstractUser | AnonymousUser, action: str) -> bool: | ||
| model = self._meta.model_name | ||
| app_label = "main" # Assume all model level permissions are in 'main' app |
There was a problem hiding this comment.
Hardcoding app_label to 'main' makes this method inflexible for models in other apps. Use self._meta.app_label instead to automatically determine the correct app label.
| app_label = "main" # Assume all model level permissions are in 'main' app | |
| app_label = self._meta.app_label # Use the actual app label of the model |
There was a problem hiding this comment.
I agree with this suggestion, since models in other apps use the BaseModel, the constructed strings will be incorrect. For example:
"create": f"{app_label}.create_{model}"
will be
"create": f"main.create_job" instead of "create": f"jobs.create_job"
ami/base/models.py
Outdated
| return ["update", "delete", "view"] | ||
|
|
||
| model = self._meta.model_name | ||
| app_label = "main" # self._meta.app_label |
There was a problem hiding this comment.
The hardcoded app_label = 'main' with a commented-out self._meta.app_label suggests this should use the model's actual app label. Use self._meta.app_label to ensure correct permissions are checked for models outside the 'main' app.
ami/base/models.py
Outdated
| """ | ||
| Retrieve collection-level permissions for the given user. | ||
| """ | ||
| app_label = "main" |
There was a problem hiding this comment.
Similar to previous comments, hardcoding app_label to 'main' limits reusability. Use cls._meta.app_label to derive the app label from the model class.
| app_label = "main" | |
| app_label = cls._meta.app_label |
| def _assign_user_permission_and_reset_caches( | ||
| self, user, perm_codename: str, app_label: str = "main", model_name: str = "project" | ||
| ): |
There was a problem hiding this comment.
This helper method is duplicated in both TestProcessingServiceModelLevelPermissions and TestTaxonModelLevelPermissions classes. Consider extracting it to a shared test base class or utility module to reduce code duplication.
| def _remove_user_permission_and_reset_cache( | ||
| self, user, perm_codename: str, app_label: str = "main", model_name: str = "project" | ||
| ): |
There was a problem hiding this comment.
This helper method is also duplicated across test classes. Extract it along with _assign_user_permission_and_reset_caches to a shared location to improve maintainability.
…t_permissions parameter order
…TestCase to remove duplication
There was a problem hiding this comment.
Hi @mohamedelabbas1996, should I see create_project permission added on the right side here? How do I add or remove the permission from a user?
Also I am not seeing the permission in the /me API response. Where is the frontend checking?
|
I found it! Under the |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a PermissionsMixin with unified object- and model-level permission APIs, makes BaseModel inherit it, removes prior per-instance BaseModel permission methods, adds serializer helper to build unsaved instances for permission checks, enforces object permissions in viewsets, introduces global roles/groups (AuthorizedUser), adds migrations, and expands permission tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant View
participant Serializer
participant PermissionsMixin
participant AuthDB
Client->>View: HTTP create/update request
View->>Serializer: validate()
alt need pre-save permission check
Serializer->>Serializer: get_instance_for_permission_check()
Serializer-->>View: unsaved instance
View->>View: check_object_permissions(request, instance)
end
View->>PermissionsMixin: check_permission(user, action)
alt object-level
PermissionsMixin->>AuthDB: get_object_level_permissions(user, object)
AuthDB-->>PermissionsMixin: object perms
else model-level
PermissionsMixin->>AuthDB: get_model_level_permissions(user, project?)
AuthDB-->>PermissionsMixin: model perms
end
View-->>Client: success or PermissionDenied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
ami/base/models.py (1)
185-198: Fix app label lookup for model-level permissions.Hardcoding
app_label = "main"means the new fallback will never honor Django’s model-level permissions for anyBaseModelsubclass outside themainapp—user.has_perm("main.create_foo")can’t match the real permission (foo.create_foo), so create/update/delete remain blocked for those models. Derive the label from the model metadata instead, and mirror the same fix in the other helpers that build permission strings (get_model_level_permissions,get_custom_model_level_permissions,get_collection_level_permissions).- app_label = "main" # Assume all model level permissions are in 'main' app + app_label = self._meta.app_labelami/main/tests.py (2)
3524-3655: Affected by BasePermissionTestCase bug.This test class calls the permission helper methods without specifying
model_name="processingservice", which means permissions are being associated with the wrong ContentType (Project instead of ProcessingService). See the critical issue raised in theBasePermissionTestCasecomment above for the fix.
3657-3799: Affected by BasePermissionTestCase bug.This test class calls the permission helper methods without specifying
model_name="taxon", which means permissions are being associated with the wrong ContentType (Project instead of Taxon). See the critical issue raised in theBasePermissionTestCasecomment above for the fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ami/base/models.py(3 hunks)ami/base/permissions.py(2 hunks)ami/base/serializers.py(1 hunks)ami/jobs/models.py(3 hunks)ami/jobs/views.py(1 hunks)ami/main/api/views.py(2 hunks)ami/main/migrations/0079_assign_authorized_user_group.py(1 hunks)ami/main/migrations/0080_alter_project_options.py(1 hunks)ami/main/migrations/0081_alter_project_options.py(1 hunks)ami/main/models.py(3 hunks)ami/main/tests.py(8 hunks)ami/ml/tests.py(1 hunks)ami/ml/views.py(4 hunks)ami/users/roles.py(6 hunks)ami/users/signals.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
ami/main/migrations/0080_alter_project_options.py (1)
ami/main/migrations/0079_assign_authorized_user_group.py (1)
Migration(85-93)
ami/base/serializers.py (1)
ami/main/api/serializers.py (1)
Meta(40-47)
ami/main/migrations/0079_assign_authorized_user_group.py (1)
ami/main/migrations/0080_alter_project_options.py (1)
Migration(6-66)
ami/main/api/views.py (1)
ami/base/permissions.py (1)
ObjectPermission(77-86)
ami/users/signals.py (1)
ami/users/roles.py (4)
AuthorizedUser(170-173)Role(12-52)assign_user(26-34)assign_user(66-73)
ami/jobs/models.py (2)
ami/main/models.py (2)
check_custom_object_level_permission(1870-1873)get_custom_object_level_permissions(1875-1892)ami/base/models.py (2)
check_custom_object_level_permission(247-256)get_custom_object_level_permissions(341-351)
ami/main/models.py (1)
ami/base/models.py (9)
check_permission(200-212)check_model_level_permission(183-198)check_object_level_permission(214-245)get_permissions(258-270)get_object_level_permissions(320-339)get_collection_level_permissions(354-370)check_custom_object_level_permission(247-256)get_project(135-151)get_custom_object_level_permissions(341-351)
ami/ml/views.py (2)
ami/base/permissions.py (1)
ObjectPermission(77-86)ami/base/serializers.py (1)
get_instance_for_permission_check(61-84)
ami/jobs/views.py (3)
ami/main/models.py (1)
check_custom_object_level_permission(1870-1873)ami/base/models.py (1)
check_custom_object_level_permission(247-256)ami/jobs/models.py (1)
check_custom_object_level_permission(952-962)
ami/base/models.py (2)
ami/main/models.py (6)
check_permission(324-337)check_permission(2202-2215)check_custom_object_level_permission(1870-1873)get_permissions(339-340)get_custom_object_level_permissions(1875-1892)get_collection_level_permissions(343-347)ami/base/serializers.py (1)
get_permissions(46-54)
ami/base/permissions.py (3)
ami/main/models.py (2)
get_permissions(339-340)get_collection_level_permissions(343-347)ami/base/models.py (2)
get_permissions(258-270)get_collection_level_permissions(354-370)ami/base/serializers.py (1)
get_permissions(46-54)
ami/users/roles.py (1)
ami/main/models.py (2)
Project(225-485)Permissions(349-418)
ami/main/tests.py (5)
ami/ml/views.py (2)
status(176-182)create(159-173)ami/tests/fixtures/main.py (1)
setup_test_project(114-131)ami/main/models.py (7)
ProjectManager(173-207)Project(225-485)Permissions(349-418)delete(2169-2200)Taxon(3243-3490)create(176-194)name(1023-1024)ami/users/roles.py (5)
ProjectManager(131-167)assign_user(26-34)assign_user(66-73)BasicMember(96-102)Identifier(109-114)ami/ml/models/processing_service.py (1)
create(28-31)
🪛 Ruff (0.14.2)
ami/main/migrations/0080_alter_project_options.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-66: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/base/serializers.py
69-69: Avoid specifying long messages outside the exception class
(TRY003)
ami/main/migrations/0079_assign_authorized_user_group.py
7-7: Unused function argument: schema_editor
(ARG001)
19-19: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
52-52: Unused function argument: schema_editor
(ARG001)
57-57: Local variable Permission is assigned to but never used
Remove assignment to unused variable Permission
(F841)
61-61: Local variable project_ct is assigned to but never used
Remove assignment to unused variable project_ct
(F841)
87-89: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
91-93: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/main/migrations/0081_alter_project_options.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-68: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/main/api/views.py
1287-1287: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/users/signals.py
78-78: Unused function argument: sender
(ARG001)
78-78: Unused function argument: kwargs
(ARG001)
ami/main/models.py
344-344: Unused class method argument: project
(ARG003)
ami/ml/views.py
146-146: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/base/models.py
326-326: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
ami/users/roles.py
15-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
173-173: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/main/tests.py
1289-1289: Possible hardcoded password assigned to argument: "password"
(S106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
ami/main/tests.py (3)
6-7: LGTM! Appropriate imports for permission testing.The addition of
PermissionandContentTypeimports is necessary for the new model-level permission test cases and follows Django's permission framework correctly.
1262-1267: LGTM! Test correctly reflects new permission model.The test rename and assertion update correctly validate that regular authenticated users can now create projects, aligning with the PR's introduction of the AuthenticatedUsers group with
create_projectpermission.
1276-1390: LGTM! Comprehensive role-based permission testing.This test class provides excellent coverage of the permission framework:
- Validates both backend Guardian permissions and API-level
user_permissionsfields- Tests all major roles (superuser, owner, manager, members, regular users)
- Uses clear helper methods to reduce duplication
- Appropriately verifies collection-level (create) vs object-level (update/delete) permissions
ami/main/api/views.py (2)
178-180: LGTM! Clear documentation of the permission flow.The updated comments effectively explain how the post_save signal assigns the ProjectManager role when both
created=Trueandowneris set. Explicitly saving with the owner ensures the signal has the necessary context for role assignment, which aligns with the model-level permissions framework introduced in this PR.
1287-1287: LGTM! Correctly adds object-level permission checks.Adding
ObjectPermissiontoTaxonViewSetaligns with the PR's objective of extending the permissions framework to support model-level permissions. SinceTaxoncan have many-to-many relationships with projects, applying object-level permission checks ensures proper authorization. This implementation is consistent with other viewsets in the file (e.g.,ProjectViewSet,DeploymentViewSet,SourceImageViewSet).Note: The static analysis hint about
typing.ClassVaris a false positive—permission_classesis a standard Django REST Framework class attribute and doesn't requireClassVarannotation.
|
Notes from our call:
|
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @mihow. * #1010 (comment) The following files were modified: * `ami/base/models.py` * `ami/base/permissions.py` * `ami/base/serializers.py` * `ami/jobs/models.py` * `ami/jobs/views.py` * `ami/main/api/views.py` * `ami/main/migrations/0079_assign_authorized_user_group.py` * `ami/main/models.py` * `ami/main/tests.py` * `ami/ml/tests.py` * `ami/ml/views.py` * `ami/users/roles.py` * `ami/users/signals.py`
…ns for creating processing services, registering pipelines, creating taxa, and assigning tags
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ami/main/models.py (1)
342-347: Consider removing or documenting the unusedprojectparameter.The
projectparameter is not used in the method body. If it's present for API consistency with other models' collection-level methods, consider adding a comment explaining this. Otherwise, it should be removed or prefixed with_to indicate it's intentionally unused.Apply this diff if the parameter is not needed:
@classmethod def get_collection_level_permissions( - cls, user: AbstractUser | AnonymousUser, project: "Project | None" = None + cls, user: AbstractUser | AnonymousUser ) -> list[str]: # Use model-level permissions for project collection-level actions return ["create"] if user.has_perm("main.create_project") else []Or add this comment if it's needed for signature consistency:
@classmethod def get_collection_level_permissions( cls, user: AbstractUser | AnonymousUser, project: "Project | None" = None ) -> list[str]: + # project parameter unused but kept for signature consistency with other models # Use model-level permissions for project collection-level actions return ["create"] if user.has_perm("main.create_project") else []ami/users/roles.py (3)
15-15: AddClassVarannotation for type safety.The
object_level_permissionsclass attribute should be annotated withtyping.ClassVarto properly indicate it's a class-level attribute that shouldn't be overridden on instances.Apply this diff:
+from typing import ClassVar + class Role: """Base class for all roles.""" - object_level_permissions = {Project.Permissions.VIEW_PROJECT} + object_level_permissions: ClassVar[set[str]] = {Project.Permissions.VIEW_PROJECT}
55-93: Consider makingGlobalRolemore generic and fix redundant import.Two issues:
Hardcoded ContentType: Line 84 hardcodes the ContentType to
Project, limitingGlobalRoleto only Project permissions. As noted in a past review comment, this should be more generic to support model-level permissions for other models.Redundant import: Line 82 imports
ContentTypeinside the method, but it's already imported at the top of the file (line 4).Consider this refactor to make the class more flexible:
@classmethod - def assign_model_level_permissions(cls, group): - from django.contrib.contenttypes.models import ContentType - - ct = ContentType.objects.get_for_model(Project) + def assign_model_level_permissions(cls, group, model_class=None): + """Assign model-level permissions to a group. + + Args: + group: The group to assign permissions to + model_class: The model class for permissions (defaults to Project) + """ + model_class = model_class or Project + ct = ContentType.objects.get_for_model(model_class) for perm_codename in cls.model_level_permissions: perm_codename = f"{perm_codename}" perm, _ = Permission.objects.get_or_create(This allows future
GlobalRolesubclasses to work with different models while maintaining backward compatibility.
173-179: AddClassVarannotation for themodel_level_permissionsattribute.Similar to the
Roleclass, themodel_level_permissionsshould be annotated withtyping.ClassVarfor better type safety.Apply this diff:
+from typing import ClassVar + class AuthorizedUser(GlobalRole): """A role that grants project create permission to all authenticated users.""" - model_level_permissions = { + model_level_permissions: ClassVar[set[str]] = { "create_project", "create_processingservice", "register_pipelines_processingservice", "create_taxon", "assign_tags_taxon", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ami/main/migrations/0081_alter_project_options.py(1 hunks)ami/main/models.py(3 hunks)ami/users/roles.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T17:19:33.225Z
Learnt from: mihow
Repo: RolnickLab/antenna PR: 1010
File: ami/users/signals.py:78-81
Timestamp: 2025-10-31T17:19:33.225Z
Learning: In the Antenna codebase (ami/users/), the AuthorizedUser group is assigned by default to newly created users but is intentionally removable by administrators. It should not be automatically re-enforced via signals after removal, as it's meant to provide flexible control over which users can create projects.
Applied to files:
ami/users/roles.py
🧬 Code graph analysis (3)
ami/users/roles.py (1)
ami/main/models.py (3)
Project(225-485)Permissions(349-418)name(1023-1024)
ami/main/migrations/0081_alter_project_options.py (1)
ami/main/migrations/0080_alter_project_options.py (1)
Migration(6-68)
ami/main/models.py (1)
ami/base/models.py (8)
check_permission(200-212)check_model_level_permission(183-198)check_object_level_permission(214-245)get_permissions(258-270)get_object_level_permissions(320-339)check_custom_object_level_permission(247-256)get_project(135-151)get_custom_object_level_permissions(341-351)
🪛 Ruff (0.14.3)
ami/users/roles.py
15-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
173-179: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/main/migrations/0081_alter_project_options.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-68: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/main/models.py
344-344: Unused class method argument: project
(ARG003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
ami/main/migrations/0081_alter_project_options.py (1)
1-68: LGTM! Migration correctly implements model-level permissions.The migration properly updates the Project model options with ordering and an extensive permissions list. The model-level permissions (lines 57-64) correctly use the "globally" suffix pattern as requested in the PR objectives.
ami/main/models.py (3)
324-337: Permission routing logic is correct for Project.The override correctly handles Project's special case where there's no project accessor. The "create" action is properly routed to model-level permission checks, while other actions use object-level checks.
1870-1892: Good refactor! Method names now clearly indicate object-level permissions.Renaming these methods from
check_custom_permissionandget_custom_user_permissionstocheck_custom_object_level_permissionandget_custom_object_level_permissionsimproves clarity and aligns with the new permission model.
2202-2215: Custom permission logic for Identification is appropriate.The override correctly implements the business rule allowing users to delete their own identifications while requiring ProjectManager role or superuser status for deleting others' identifications.
ami/users/roles.py (2)
170-179:AuthorizedUsercorrectly implements global permissions.The
AuthorizedUserrole appropriately grants model-level permissions for creating projects, processing services, taxons, and related operations. This aligns with the PR objectives to enable authenticated users to create projects globally.Based on learnings: The AuthorizedUser group is intentionally removable by administrators for flexible control.
96-167: Role refactoring improves clarity and composability.The changes to use explicit
object_level_permissionsand compose permissions through set operations (e.g.,BasicMember.object_level_permissions | {...}) make the permission inheritance model clearer and more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
ami/base/permissions.py (4)
106-123: Critical: Hardcodedapp_label = "main"breaks permissions for models in other apps.BaseModel is used by models across multiple apps (jobs, ml, etc.), but this method hardcodes
app_label = "main". Django registers permissions per-app, so a Job model's permissions are under "jobs", not "main". This will cause all permission checks to fail for non-main models.Apply this diff to use the model's actual app label:
def check_model_level_permission( self: BaseModel, user: AbstractUser | AnonymousUser, action: str # type: ignore[override] ) -> bool: model = self._meta.model_name - app_label = "main" # Assume all model level permissions are in 'main' app + app_label = self._meta.app_label crud_map = { "create": f"{app_label}.create_{model}",Based on past review comments.
201-225: Critical: Hardcodedapp_label = "main"breaks model-level permissions.Same issue as
check_model_level_permission: the hardcoded "main" will cause permission lookups to fail for models in other apps like jobs or ml.Apply this diff:
def get_model_level_permissions( self: BaseModel, user: AbstractUser | AnonymousUser # type: ignore[override] ) -> list[str]: """ Retrieve model-level permissions for the given user. Returns a list of allowed actions such as ["create", "update", "delete"]. """ if user.is_superuser: # Superusers get all possible actions return ["update", "delete", "view"] model = self._meta.model_name - app_label = "main" # self._meta.app_label + app_label = self._meta.app_labelBased on past review comments.
227-251: Critical: Hardcodedapp_label = "main"in custom permission retrieval.Custom model-level permissions for non-main models will be missed due to the hardcoded "main" filter.
Apply this diff:
def get_custom_model_level_permissions( self: BaseModel, user: AbstractUser | AnonymousUser # type: ignore[override] ) -> list[str]: """ Retrieve custom (non-CRUD) model-level permissions for the given user. Custom permissions follow the pattern: <app_label>.<custom_action>_<model_name> Example: "main.register_pipelines_processingservice" """ model = self._meta.model_name - app_label = "main" + app_label = self._meta.app_labelBased on past review comments.
290-309: Critical: Hardcodedapp_label = "main"in collection-level permissions.The classmethod also hardcodes "main", which breaks create permission checks for models in other apps.
Apply this diff:
@classmethod def get_collection_level_permissions( cls: type[BaseModel], user: AbstractUser | AnonymousUser, project # type: ignore ) -> list[str]: """ Retrieve collection-level permissions for the given user. """ - app_label = "main" + app_label = cls._meta.app_labelBased on past review comments.
ami/main/tests.py (1)
3485-3520: Critical: Defaultmodel_name="project"causes incorrect ContentType association.The helper methods default
model_name="project", but they're called without overriding this parameter when assigning permissions for other models (e.g., lines 3572, 3761 use "create_processingservice" and "create_taxon"). This causesContentType.objects.get(model="project")to be used for all permissions, associating them with the wrong ContentType.Impact: Tests may pass incorrectly because permissions are associated with the Project model instead of the intended model (ProcessingService, Taxon, etc.), masking real permission failures in production.
Apply this diff to derive the model name from the permission codename:
def _assign_user_permission_and_reset_caches( - self, user, perm_codename: str, app_label: str = "main", model_name: str = "project" + self, user, perm_codename: str, app_label: str = "main", model_name: str | None = None ): + # Derive model name from permission codename if not explicitly provided + # E.g., "create_processingservice" -> "processingservice" + if model_name is None: + # Remove common action prefixes to get model name + for prefix in ["create_", "update_", "delete_", "view_", "register_pipelines_", "assign_tags_"]: + if perm_codename.startswith(prefix): + model_name = perm_codename[len(prefix):] + break + else: + # Fallback: use the entire codename as model name + model_name = perm_codename + ct = ContentType.objects.get(app_label=app_label, model=model_name) perm, _ = Permission.objects.get_or_create( codename=perm_codename, content_type=ct, defaults={"name": f"Can {perm_codename.replace('_', ' ')}"}, ) user.user_permissions.add(perm) # Clear cached permissions for attr in ["_perm_cache", "_user_perm_cache", "_group_perm_cache"]: if hasattr(user, attr): delattr(user, attr) user.refresh_from_db() return perm def _remove_user_permission_and_reset_cache( - self, user, perm_codename: str, app_label: str = "main", model_name: str = "project" + self, user, perm_codename: str, app_label: str = "main", model_name: str | None = None ): + # Derive model name from permission codename if not explicitly provided + if model_name is None: + for prefix in ["create_", "update_", "delete_", "view_", "register_pipelines_", "assign_tags_"]: + if perm_codename.startswith(prefix): + model_name = perm_codename[len(prefix):] + break + else: + model_name = perm_codename + try: ct = ContentType.objects.get(app_label=app_label, model=model_name) perm = Permission.objects.get(codename=perm_codename, content_type=ct) user.user_permissions.remove(perm) except Permission.DoesNotExist: return False # Clear cached permissions for attr in ["_perm_cache", "_user_perm_cache", "_group_perm_cache"]: if hasattr(user, attr): delattr(user, attr) user.refresh_from_db() return TrueBased on past review comments.
🧹 Nitpick comments (2)
ami/users/signals.py (1)
77-81: LGTM! Consider adding defensive error handling.The signal correctly assigns the AuthorizedUser role only to newly created users, and the implementation aligns with the intended behavior (the role is a default for new users but remains removable by administrators). The logging is clear and helpful.
The static analysis warnings about unused
senderandkwargsare false positives—these parameters are required by Django's signal receiver signature and follow the standard pattern for forward compatibility.As an optional improvement, consider wrapping the role assignment in a try-except block to prevent user creation from failing if role assignment encounters a database error:
@receiver(post_save, sender=User) def assign_authorized_user_group(sender, instance, created, **kwargs): if created: - logger.info(f"Assigning AuthorizedUser role to new user {instance.email}") - AuthorizedUser.assign_user(instance) + try: + logger.info(f"Assigning AuthorizedUser role to new user {instance.email}") + AuthorizedUser.assign_user(instance) + except Exception as e: + logger.error(f"Failed to assign AuthorizedUser role to {instance.email}: {e}")This ensures user registration succeeds even if the role assignment fails, allowing the issue to be resolved separately.
ami/base/permissions.py (1)
260-261: Optional: Consider iterable unpacking instead of list concatenation.The concatenation with
+works but iterable unpacking is more idiomatic.Apply this diff:
if user.is_superuser: - return ["update", "delete"] + self.get_custom_object_level_permissions(user) + return [*["update", "delete"], *self.get_custom_object_level_permissions(user)]Or more concisely:
if user.is_superuser: - return ["update", "delete"] + self.get_custom_object_level_permissions(user) + return ["update", "delete", *self.get_custom_object_level_permissions(user)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ami/base/models.py(2 hunks)ami/base/permissions.py(3 hunks)ami/main/tests.py(8 hunks)ami/users/signals.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T17:19:33.252Z
Learnt from: mihow
Repo: RolnickLab/antenna PR: 1010
File: ami/users/signals.py:78-81
Timestamp: 2025-10-31T17:19:33.252Z
Learning: In the Antenna codebase (ami/users/), the AuthorizedUser group is assigned by default to newly created users but is intentionally removable by administrators. It should not be automatically re-enforced via signals after removal, as it's meant to provide flexible control over which users can create projects.
Applied to files:
ami/users/signals.py
🧬 Code graph analysis (4)
ami/base/permissions.py (5)
ami/base/models.py (3)
BaseModel(91-170)get_project(135-151)get_project_accessor(99-133)ami/base/serializers.py (1)
get_permissions(46-54)ami/main/models.py (6)
get_permissions(339-340)get_collection_level_permissions(343-347)check_permission(324-337)check_permission(2202-2215)check_custom_object_level_permission(1870-1873)get_custom_object_level_permissions(1875-1892)ami/jobs/models.py (3)
logger(991-1000)check_custom_object_level_permission(952-962)get_custom_object_level_permissions(964-983)ami/users/roles.py (2)
BasicMember(96-102)has_role(44-47)
ami/base/models.py (1)
ami/base/permissions.py (1)
PermissionsMixin(80-309)
ami/main/tests.py (5)
ami/ml/views.py (2)
status(176-182)create(159-173)ami/tests/fixtures/main.py (1)
setup_test_project(114-131)ami/main/models.py (7)
ProjectManager(173-207)Project(225-485)Permissions(349-418)name(1023-1024)delete(2169-2200)Taxon(3243-3490)create(176-194)ami/users/roles.py (5)
ProjectManager(131-167)assign_user(26-34)assign_user(66-73)BasicMember(96-102)Identifier(109-114)ami/ml/models/processing_service.py (1)
create(28-31)
ami/users/signals.py (1)
ami/users/roles.py (4)
AuthorizedUser(170-179)Role(12-52)assign_user(26-34)assign_user(66-73)
🪛 Ruff (0.14.3)
ami/base/permissions.py
86-86: Docstring contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF002)
261-261: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
ami/main/tests.py
1289-1289: Possible hardcoded password assigned to argument: "password"
(S106)
ami/users/signals.py
78-78: Unused function argument: sender
(ARG001)
78-78: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (9)
ami/users/signals.py (1)
5-10: LGTM!The import additions are necessary for the new signal receiver and follow Django conventions.
ami/base/models.py (2)
1-6: LGTM: Clean import additions for the permission refactor.The new imports support the PermissionsMixin integration. The changes are minimal and correct.
91-91: LGTM: Correct mixin inheritance order.The inheritance order
PermissionsMixin, models.Modelis correct for Python's MRO (Method Resolution Order), ensuring the mixin's permission methods are available to all BaseModel subclasses.ami/base/permissions.py (1)
4-11: LGTM: Proper lazy import pattern for circular dependency.Using
TYPE_CHECKINGto lazily importBaseModelavoids circular import issues while preserving type hints. This is the correct pattern.ami/main/tests.py (5)
6-7: LGTM: Necessary imports for permission test helpers.The Permission and ContentType imports enable programmatic permission management in the new test classes.
1262-1267: LGTM: Test updated to reflect new permission model.The test correctly reflects that regular users can now create projects with the AuthorizedUser global role, changing the expected status from 403 to 201.
1276-1390: LGTM: Comprehensive role-based permission verification.This test class thoroughly validates that guardian backend permissions match API user_permissions fields across different roles (superuser, owner, project manager, basic member, identifier, regular user). The helper methods comparing backend and API responses are well-structured.
3536-3714: Comprehensive model-level permission tests for ProcessingService.These tests thoroughly verify that create, update, delete, and custom action (register_pipelines) permissions work correctly at the model level and are properly reflected in API responses. However, the helper method bug (lines 3485-3520) must be fixed for these tests to be valid.
3716-3909: Comprehensive model-level permission tests for Taxon.Similar to ProcessingService tests, these verify CRUD and custom action (assign_tags) permissions. Again, the helper method ContentType bug must be addressed for accurate test results.
| class PermissionsMixin: | ||
| """ | ||
| A mixin for `BaseModel` that provides methods to check and retrieve | ||
| both object-level and model-level permissions, supporting standard | ||
| CRUD actions as well as custom project-specific permissions. | ||
| It integrates with Django’s permission framework and Django Guardian | ||
| to handle global (model-level) and project-scoped (object-level) access | ||
| control. This allows consistent permission checks across all models | ||
| that inherit from `BaseModel`. | ||
| """ |
There was a problem hiding this comment.
Minor: Docstring contains curly quote instead of backtick.
The docstring uses a RIGHT SINGLE QUOTATION MARK (') instead of a GRAVE ACCENT (`).
Apply this diff:
"""
A mixin for `BaseModel` that provides methods to check and retrieve
both object-level and model-level permissions, supporting standard
CRUD actions as well as custom project-specific permissions.
- It integrates with Django's permission framework and Django Guardian
+ It integrates with Django's permission framework and Django Guardian
to handle global (model-level) and project-scoped (object-level) access
control. This allows consistent permission checks across all models
that inherit from `BaseModel`.
"""📝 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.
| class PermissionsMixin: | |
| """ | |
| A mixin for `BaseModel` that provides methods to check and retrieve | |
| both object-level and model-level permissions, supporting standard | |
| CRUD actions as well as custom project-specific permissions. | |
| It integrates with Django’s permission framework and Django Guardian | |
| to handle global (model-level) and project-scoped (object-level) access | |
| control. This allows consistent permission checks across all models | |
| that inherit from `BaseModel`. | |
| """ | |
| class PermissionsMixin: | |
| """ | |
| A mixin for `BaseModel` that provides methods to check and retrieve | |
| both object-level and model-level permissions, supporting standard | |
| CRUD actions as well as custom project-specific permissions. | |
| It integrates with Django's permission framework and Django Guardian | |
| to handle global (model-level) and project-scoped (object-level) access | |
| control. This allows consistent permission checks across all models | |
| that inherit from `BaseModel`. | |
| """ |
🧰 Tools
🪛 Ruff (0.14.3)
86-86: Docstring contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF002)
🤖 Prompt for AI Agents
In ami/base/permissions.py around lines 80 to 90, the class docstring uses a
RIGHT SINGLE QUOTATION MARK (’) instead of a backtick (`) around BaseModel;
replace the curly quote with a grave accent so it reads `BaseModel` (and scan
the docstring for any other curly quotes and convert them to straight backticks
where code identifiers are intended).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ami/main/tests.py (1)
3485-3520: Critical: Defaultmodel_nameparameter causes incorrect ContentType lookup.The helper methods default
model_name="project", but they're used to assign permissions for other models (ProcessingService at line 3572, Taxon at line 3761) without overriding this parameter. This causesContentType.objects.get(model="project")to be used for all models, associating permissions with the wrong content type.Impact: Tests may pass, but permissions are associated with the Project model instead of the intended models (ProcessingService, Taxon), causing permission checks to fail in production.
Apply the fix suggested in the past review comments to derive
model_namefrom theperm_codenamewhen not explicitly provided:def _assign_user_permission_and_reset_caches( - self, user, perm_codename: str, app_label: str = "main", model_name: str = "project" + self, user, perm_codename: str, app_label: str = "main", model_name: str | None = None ): + # Derive model name from permission codename if not explicitly provided + # E.g., "create_processingservice" -> "processingservice" + if model_name is None: + # Remove common action prefixes to get model name + for prefix in ["create_", "update_", "delete_", "view_"]: + if perm_codename.startswith(prefix): + model_name = perm_codename[len(prefix):] + break + else: + # Fallback: use the entire codename as model name + model_name = perm_codename + ct = ContentType.objects.get(app_label=app_label, model=model_name) perm, _ = Permission.objects.get_or_create( codename=perm_codename, content_type=ct, defaults={"name": f"Can {perm_codename.replace('_', ' ')}"}, ) user.user_permissions.add(perm) # Clear cached permissions for attr in ["_perm_cache", "_user_perm_cache", "_group_perm_cache"]: if hasattr(user, attr): delattr(user, attr) user.refresh_from_db() return perm def _remove_user_permission_and_reset_cache( - self, user, perm_codename: str, app_label: str = "main", model_name: str = "project" + self, user, perm_codename: str, app_label: str = "main", model_name: str | None = None ): + # Derive model name from permission codename if not explicitly provided + if model_name is None: + for prefix in ["create_", "update_", "delete_", "view_"]: + if perm_codename.startswith(prefix): + model_name = perm_codename[len(prefix):] + break + else: + model_name = perm_codename + try: ct = ContentType.objects.get(app_label=app_label, model=model_name) perm = Permission.objects.get(codename=perm_codename, content_type=ct) user.user_permissions.remove(perm) except Permission.DoesNotExist: return False # Clear cached permissions for attr in ["_perm_cache", "_user_perm_cache", "_group_perm_cache"]: if hasattr(user, attr): delattr(user, attr) user.refresh_from_db() return True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ami/main/tests.py(8 hunks)ami/ml/tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ami/ml/tests.py
🧰 Additional context used
🧬 Code graph analysis (1)
ami/main/tests.py (6)
ami/ml/views.py (2)
status(176-182)create(159-173)ami/users/managers.py (2)
create_user(21-24)create_superuser(26-35)ami/tests/fixtures/main.py (1)
setup_test_project(114-131)ami/main/models.py (7)
ProjectManager(173-207)Project(225-485)Permissions(349-418)name(1023-1024)delete(2169-2200)Taxon(3243-3490)create(176-194)ami/users/roles.py (5)
ProjectManager(131-167)assign_user(26-34)assign_user(66-73)BasicMember(96-102)Identifier(109-114)ami/ml/models/processing_service.py (1)
create(29-32)
🪛 Ruff (0.14.3)
ami/main/tests.py
1289-1289: Possible hardcoded password assigned to argument: "password"
(S106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: test
🔇 Additional comments (3)
ami/main/tests.py (3)
6-7: LGTM! Necessary imports for permission testing.These imports support the new model-level permission testing infrastructure introduced in this PR.
1262-1267: Correctly reflects new permission model.The test rename and assertion change align with the PR's introduction of the AuthorizedUser global role, which grants all authenticated users the
create_projectpermission. The test now validates that regular users can successfully create projects.
1276-1390: Excellent comprehensive test coverage for role-based permissions.This test class thoroughly validates that:
- Backend guardian permissions match API
user_permissionsfields- Collection-level (create) and object-level (update, delete) permissions are correctly exposed
- All user roles (superuser, owner, project_manager, basic_member, identifier, regular_user) have appropriate permissions
The helper methods (
_assert_permissions_match_api,_get_project_object,_get_project_list) promote maintainability and reduce duplication.
| class TestProcessingServiceModelLevelPermissions(BasePermissionTestCase): | ||
| """ | ||
| Tests model-level permissions for ProcessingService model. | ||
| Ensures that create, update, and delete actions are controlled by model-level permissions | ||
| and correctly reflected in the API responses. | ||
| """ | ||
|
|
||
| def setUp(self): | ||
| self.user = User.objects.create_user( | ||
| email="perm_tester@insectai.org", | ||
| is_staff=False, | ||
| is_superuser=False, | ||
| ) | ||
| self.project, _ = setup_test_project(reuse=False) | ||
| self.client.force_authenticate(user=self.user) | ||
| # Remove the user from the AuthorizedUsers group to avoid inherited permissions | ||
| self._remove_user_from_authorized_group(self.user) | ||
|
|
||
| self.endpoint = "/api/v2/ml/processing_services/" | ||
| self.payload = { | ||
| "name": "Test Processing Service", | ||
| "description": "For permission testing", | ||
| "endpoint_url": "http://processing_service:2000", | ||
| "project": self.project.pk, | ||
| } | ||
|
|
||
| def test_create_requires_model_level_permission(self): | ||
| """User cannot create ProcessingService without model-level create permission.""" | ||
| response = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_403_FORBIDDEN, | ||
| "User without create_processingservice permission should not be able to create a ProcessingService", | ||
| ) | ||
|
|
||
| # Grant model-level create permission | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_processingservice") | ||
| response = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_201_CREATED, | ||
| "User with create_processingservice permission should be able to create a ProcessingService", | ||
| ) | ||
|
|
||
| def test_update_requires_model_level_permission(self): | ||
| """User cannot update ProcessingService without model-level update permission.""" | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_processingservice") | ||
| create_resp = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual(create_resp.status_code, 201) | ||
| service_id = create_resp.data["instance"]["id"] | ||
|
|
||
| update_url = f"{self.endpoint}{service_id}/" | ||
|
|
||
| # Remove create permission | ||
| self._remove_user_permission_and_reset_cache(self.user, "create_processingservice") | ||
| response = self.client.patch(update_url, {"description": "Updated"}, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_403_FORBIDDEN, | ||
| "User without update_processingservice permission should not be able to update a ProcessingService", | ||
| ) | ||
|
|
||
| # Grant update permission | ||
| self._assign_user_permission_and_reset_caches(self.user, "update_processingservice") | ||
| response = self.client.patch(update_url, {"description": "Updated Description"}, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_200_OK, | ||
| "User with update_processingservice permission should be able to update a ProcessingService", | ||
| ) | ||
|
|
||
| def test_delete_requires_model_level_permission(self): | ||
| """User cannot delete ProcessingService without model-level delete permission.""" | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_processingservice") | ||
| create_resp = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual(create_resp.status_code, 201) | ||
| service_id = create_resp.data["instance"]["id"] | ||
| delete_url = f"{self.endpoint}{service_id}/" | ||
|
|
||
| # No delete permission yet | ||
| response = self.client.delete(delete_url) | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_403_FORBIDDEN, | ||
| "User without delete_processingservice permission should not be able to delete a ProcessingService", | ||
| ) | ||
|
|
||
| # Grant delete permission | ||
| self._assign_user_permission_and_reset_caches(self.user, "delete_processingservice") | ||
| response = self.client.delete(delete_url) | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_204_NO_CONTENT, | ||
| "User with delete_processingservice permission should be able to delete a ProcessingService", | ||
| ) | ||
|
|
||
| def test_permissions_reflected_in_collection_user_permissions(self): | ||
| """ | ||
| Verify that model-level permissions (create, update, delete) | ||
| appear correctly in the API responses at both collection and object levels. | ||
| """ | ||
| # Grant all model-level permissions | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_processingservice") | ||
| self._assign_user_permission_and_reset_caches(self.user, "update_processingservice") | ||
| self._assign_user_permission_and_reset_caches(self.user, "delete_processingservice") | ||
| # Create one instance | ||
| response = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual(response.status_code, 201) | ||
| service_id = response.data["instance"]["id"] | ||
|
|
||
| # Check collection-level permissions | ||
| list_resp = self.client.get(self.endpoint) | ||
| self.assertEqual(list_resp.status_code, 200) | ||
| collection_perms = list_resp.data.get("user_permissions", []) | ||
|
|
||
| self.assertIn( | ||
| "create", | ||
| collection_perms, | ||
| "create permission should appear in collection-level user_permissions for ProcessingService", | ||
| ) | ||
|
|
||
| # Check object-level user_permissions in results | ||
| found_obj = next( | ||
| (item for item in list_resp.data.get("results", []) if item["id"] == service_id), | ||
| None, | ||
| ) | ||
| self.assertIsNotNone(found_obj, "ProcessingService should appear in list results") | ||
| for perm in ["update", "delete"]: | ||
| self.assertIn( | ||
| perm, | ||
| found_obj.get("user_permissions", []), | ||
| f"'{perm}' should appear in object-level user_permissions for ProcessingService", | ||
| ) | ||
|
|
||
| def test_register_pipelines_custom_action_requires_permission(self): | ||
| """ | ||
| Verify that the custom 'register_pipelines' action requires the corresponding | ||
| model-level permission and appears in user_permissions after granting it. | ||
| """ | ||
| # Grant create permission to allow instance creation | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_processingservice") | ||
| create_resp = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual(create_resp.status_code, 201) | ||
| service_id = create_resp.data["instance"]["id"] | ||
|
|
||
| # Try performing the custom action WITHOUT permission | ||
| action_url = f"{self.endpoint}{service_id}/register_pipelines/" | ||
| response = self.client.post(action_url, {}, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_403_FORBIDDEN, | ||
| "User without 'register_pipelines_processingservice' permission should not be able to register pipelines.", | ||
| ) | ||
|
|
||
| # Grant the custom permission | ||
| self._assign_user_permission_and_reset_caches(self.user, "register_pipelines_processingservice") | ||
|
|
||
| # Retry performing the custom action — should now be allowed | ||
| response = self.client.post(action_url, {}, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_200_OK, | ||
| "User with 'register_pipelines_processingservice' permission should be able to register pipelines.", | ||
| ) | ||
|
|
||
| # Fetch list and confirm custom permission now appears for this object | ||
| list_resp = self.client.get(self.endpoint) | ||
| self.assertEqual(list_resp.status_code, 200) | ||
| found_obj = next( | ||
| (item for item in list_resp.data.get("results", []) if item["id"] == service_id), | ||
| None, | ||
| ) | ||
| self.assertIsNotNone(found_obj, "ProcessingService should appear in list results.") | ||
| self.assertIn( | ||
| "register_pipelines", | ||
| found_obj.get("user_permissions", []), | ||
| "Custom permission 'register_pipelines' should appear in object-level user_permissions after granting it.", | ||
| ) | ||
|
|
There was a problem hiding this comment.
Well-structured model-level permission tests, but blocked by critical base class bug.
This test class provides excellent coverage:
- CRUD operations require appropriate model-level permissions
- Custom action
register_pipelinesrequires specific permission - API responses correctly reflect user permissions at collection and object levels
However, these tests are affected by the critical bug in BasePermissionTestCase helper methods (lines 3485-3520). The default model_name="project" causes permissions to be associated with the wrong ContentType, so these tests may pass incorrectly.
Please address the base class bug first, then verify these tests still pass with the corrected ContentType lookups.
| class TestTaxonModelLevelPermissions(BasePermissionTestCase): | ||
| """ | ||
| Tests model-level permissions for Taxon. | ||
| Ensures that create, update, and delete actions are controlled by model-level permissions | ||
| and correctly reflected in the API responses. | ||
| """ | ||
|
|
||
| def setUp(self): | ||
| self.user = User.objects.create_user( | ||
| email="perm_tester@insectai.org", | ||
| is_staff=False, | ||
| is_superuser=False, | ||
| ) | ||
| # Remove the user from the AuthorizedUsers group to avoid inherited permissions | ||
| self._remove_user_from_authorized_group(self.user) | ||
| self.project, _ = setup_test_project(reuse=False) | ||
| self.client.force_authenticate(user=self.user) | ||
|
|
||
| self.endpoint = "/api/v2/taxa/" | ||
| self.parent_taxon = Taxon.objects.create(name="ParentTaxon", rank="GENUS") | ||
|
|
||
| self.payload = { | ||
| "name": "Test Taxon", | ||
| "rank": "SPECIES", | ||
| "parent_id": self.parent_taxon.pk, | ||
| "project": self.project.pk, | ||
| } | ||
|
|
||
| # ---------- helpers ---------- | ||
| def _add_taxon_to_project(self, taxon_id: int): | ||
| taxon = Taxon.objects.get(pk=taxon_id) | ||
| taxon.projects.add(self.project) | ||
| taxon.save() | ||
|
|
||
| # ---------- tests ---------- | ||
| def test_create_requires_model_level_permission(self): | ||
| """User cannot create Taxon without model-level create permission.""" | ||
| response = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_403_FORBIDDEN, | ||
| "User without create_taxon permission should not be able to create a Taxon", | ||
| ) | ||
|
|
||
| # Grant model-level create permission | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_taxon") | ||
| response = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_201_CREATED, | ||
| "User with create_taxon permission should be able to create a Taxon", | ||
| ) | ||
| # Remove the created taxon object | ||
| taxon_id = response.data["id"] | ||
| taxon = Taxon.objects.get(pk=taxon_id) | ||
| taxon.delete() | ||
|
|
||
| def test_update_requires_model_level_permission(self): | ||
| """User cannot update Taxon without model-level update permission.""" | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_taxon") | ||
| create_resp = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual(create_resp.status_code, 201) | ||
| taxon_id = create_resp.data["id"] | ||
| self._add_taxon_to_project(taxon_id) | ||
| update_url = f"{self.endpoint}{taxon_id}/" | ||
|
|
||
| # Remove create permission | ||
| self._remove_user_permission_and_reset_cache(self.user, "create_taxon") | ||
| response = self.client.patch(update_url, {"name": "Updated Taxon"}, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_403_FORBIDDEN, | ||
| "User without update_taxon permission should not be able to update a Taxon", | ||
| ) | ||
|
|
||
| # Grant update permission | ||
| self._assign_user_permission_and_reset_caches(self.user, "update_taxon") | ||
| response = self.client.patch(update_url, {"name": "Updated Taxon"}, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_200_OK, | ||
| "User with update_taxon permission should be able to update a Taxon", | ||
| ) | ||
|
|
||
| def test_delete_requires_model_level_permission(self): | ||
| """User cannot delete Taxon without model-level delete permission.""" | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_taxon") | ||
| create_resp = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual(create_resp.status_code, 201) | ||
| taxon_id = create_resp.data["id"] | ||
| self._add_taxon_to_project(taxon_id) | ||
| delete_url = f"{self.endpoint}{taxon_id}/" | ||
|
|
||
| # No delete permission yet | ||
| response = self.client.delete(delete_url) | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_403_FORBIDDEN, | ||
| "User without delete_taxon permission should not be able to delete a Taxon", | ||
| ) | ||
|
|
||
| # Grant delete permission | ||
| self._assign_user_permission_and_reset_caches(self.user, "delete_taxon") | ||
| response = self.client.delete(delete_url) | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_204_NO_CONTENT, | ||
| "User with delete_taxon permission should be able to delete a Taxon", | ||
| ) | ||
|
|
||
| def test_permissions_reflected_in_collection_user_permissions(self): | ||
| """ | ||
| Verify that model-level permissions (create, update, delete) | ||
| appear correctly in the API responses at both collection and object levels. | ||
| """ | ||
| # Grant all model-level permissions | ||
| for perm in ["create_taxon", "update_taxon", "delete_taxon"]: | ||
| self._assign_user_permission_and_reset_caches(self.user, perm) | ||
|
|
||
| # Create one instance | ||
| create_resp = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual(create_resp.status_code, 201) | ||
| taxon_id = create_resp.data["id"] | ||
| self._add_taxon_to_project(taxon_id) | ||
|
|
||
| # Check collection-level permissions | ||
| list_resp = self.client.get(self.endpoint) | ||
| self.assertEqual(list_resp.status_code, 200) | ||
| collection_perms = list_resp.data.get("user_permissions", []) | ||
| self.assertIn( | ||
| "create", collection_perms, "create permission should appear in collection-level user_permissions" | ||
| ) | ||
|
|
||
| # Check object-level user_permissions in results | ||
| found_obj = next( | ||
| (item for item in list_resp.data.get("results", []) if item["id"] == taxon_id), | ||
| None, | ||
| ) | ||
| self.assertIsNotNone(found_obj, "Taxon should appear in list results") | ||
| for perm in ["update", "delete"]: | ||
| self.assertIn( | ||
| perm, | ||
| found_obj.get("user_permissions", []), | ||
| f"'{perm}' should appear in object-level user_permissions for Taxon", | ||
| ) | ||
|
|
||
| def test_assign_tags_custom_action_requires_permission(self): | ||
| """ | ||
| Verify that the custom 'assign_tags' action requires the corresponding | ||
| model-level permission and appears in user_permissions after granting it. | ||
| """ | ||
| # Grant create permission to create a taxon | ||
| self._assign_user_permission_and_reset_caches(self.user, "create_taxon") | ||
| create_resp = self.client.post(self.endpoint, self.payload, format="json") | ||
| self.assertEqual(create_resp.status_code, 201) | ||
| taxon_id = create_resp.data["id"] | ||
| self._add_taxon_to_project(taxon_id) | ||
| # Prepare custom action URL | ||
| action_url = f"{self.endpoint}{taxon_id}/assign_tags/" | ||
| tag = Tag.objects.create(name="test-tag", project=self.project) | ||
| payload = {"tag_ids": [tag.pk]} | ||
|
|
||
| # Try performing the custom action WITHOUT permission | ||
| response = self.client.post(action_url, payload, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_403_FORBIDDEN, | ||
| "User without 'assign_tags_taxon' permission should not be able to assign tags to a Taxon.", | ||
| ) | ||
|
|
||
| # Grant the custom permission | ||
| self._assign_user_permission_and_reset_caches(self.user, "assign_tags_taxon") | ||
|
|
||
| # Retry performing the action — should now succeed | ||
| response = self.client.post(action_url, payload, format="json") | ||
| self.assertEqual( | ||
| response.status_code, | ||
| status.HTTP_200_OK, | ||
| "User with 'assign_tags_taxon' permission should be able to assign tags to a Taxon.", | ||
| ) | ||
|
|
||
| # Fetch list and confirm permission appears on the object | ||
| list_resp = self.client.get(self.endpoint) | ||
| self.assertEqual(list_resp.status_code, 200) | ||
| found_obj = next( | ||
| (item for item in list_resp.data.get("results", []) if item["id"] == taxon_id), | ||
| None, | ||
| ) | ||
| self.assertIsNotNone(found_obj, "Taxon should appear in list results.") | ||
| self.assertIn( | ||
| "assign_tags", | ||
| found_obj.get("user_permissions", []), | ||
| "Custom permission 'assign_tags' should appear in object-level user_permissions after granting it.", | ||
| ) |
There was a problem hiding this comment.
Well-structured model-level permission tests for Taxon, but blocked by critical base class bug.
This test class mirrors the ProcessingService tests and provides comprehensive coverage:
- CRUD operations require appropriate model-level permissions
- Custom action
assign_tagsrequires specific permission - API responses correctly reflect user permissions
- Helper
_add_taxon_to_projectappropriately handles taxon-project associations
However, these tests are also affected by the critical bug in BasePermissionTestCase helper methods (lines 3485-3520). The default model_name="project" will cause permissions to be associated with the Project ContentType instead of Taxon, leading to incorrect test results.
Please address the base class bug first, then verify these tests still pass with the corrected ContentType lookups.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
ami/main/tests.py (3)
3479-3534: Critical: Helper methods have incorrect defaultmodel_nameparameter.The helper methods default
model_name="project", but they are used throughout the test suite to assign permissions for other models like ProcessingService and Taxon. This causesContentType.objects.get(model="project")to be used for all models, associating permissions with the wrong content type.Impact: Tests may pass incorrectly because permissions are associated with the Project model instead of the intended model (ProcessingService, Taxon, etc.), causing permission checks to fail in production.
Apply this diff to derive the model name from the permission codename:
def _assign_user_permission_and_reset_caches( - self, user, perm_codename: str, app_label: str = "main", model_name: str = "project" + self, user, perm_codename: str, app_label: str = "main", model_name: str | None = None ): + # Derive model name from permission codename if not explicitly provided + if model_name is None: + # Extract model name from codename (e.g., "create_processingservice" -> "processingservice") + for prefix in ["create_", "update_", "delete_", "view_", "register_pipelines_", "assign_tags_"]: + if perm_codename.startswith(prefix): + model_name = perm_codename[len(prefix):] + break + else: + # Fallback to "project" if no prefix matches + model_name = "project" + ct = ContentType.objects.get(app_label=app_label, model=model_name) perm, _ = Permission.objects.get_or_create( codename=perm_codename, content_type=ct, defaults={"name": f"Can {perm_codename.replace('_', ' ')}"}, ) user.user_permissions.add(perm) # Clear cached permissions for attr in ["_perm_cache", "_user_perm_cache", "_group_perm_cache"]: if hasattr(user, attr): delattr(user, attr) user.refresh_from_db() return perm def _remove_user_permission_and_reset_cache( - self, user, perm_codename: str, app_label: str = "main", model_name: str = "project" + self, user, perm_codename: str, app_label: str = "main", model_name: str | None = None ): + # Derive model name from permission codename if not explicitly provided + if model_name is None: + for prefix in ["create_", "update_", "delete_", "view_", "register_pipelines_", "assign_tags_"]: + if perm_codename.startswith(prefix): + model_name = perm_codename[len(prefix):] + break + else: + model_name = "project" + try: ct = ContentType.objects.get(app_label=app_label, model=model_name) perm = Permission.objects.get(codename=perm_codename, content_type=ct) user.user_permissions.remove(perm) except Permission.DoesNotExist: return False # Clear cached permissions for attr in ["_perm_cache", "_user_perm_cache", "_group_perm_cache"]: if hasattr(user, attr): delattr(user, attr) user.refresh_from_db() return True
3536-3714: Well-structured model-level permission tests, but blocked by critical base class bug.This test class provides comprehensive coverage of ProcessingService permissions:
- CRUD operations require appropriate model-level permissions
- Custom
register_pipelinesaction requires specific permission- Permissions are correctly reflected in API responses at collection and object levels
However, these tests are affected by the critical bug in
BasePermissionTestCasehelper methods (lines 3479-3520). Please address the base class bug first, then verify these tests still pass.
3716-3909: Well-structured model-level permission tests for Taxon, but blocked by critical base class bug.This test class mirrors the ProcessingService tests and provides comprehensive coverage:
- CRUD operations require appropriate model-level permissions
- Custom
assign_tagsaction requires specific permission- API responses correctly reflect user permissions
- Helper
_add_taxon_to_projectappropriately handles taxon-project associationsHowever, these tests are also affected by the critical bug in
BasePermissionTestCasehelper methods (lines 3479-3520). Please address the base class bug first, then verify these tests still pass.
🧹 Nitpick comments (2)
ami/users/roles.py (2)
55-125: LGTM! Well-designed GlobalRole class for model-level permissions.The GlobalRole class provides a clean abstraction for model-level permissions with:
- Group management methods (assign_user, unassign_user)
- Permission synchronization via
sync_group_permissionsto handle role evolution- Proper error handling and logging
The hardcoded
ContentType.objects.get_for_model(Project)at line 84 is intentional and correct—all model-level permissions in this system are defined on the Project model (perProject.Meta.permissions), includingcreate_processingservice,create_taxon, etc.Optional: Consider annotating class attributes with
typing.ClassVarto satisfy static analysis (lines 58, as flagged by Ruff). This is a minor style improvement:+from typing import ClassVar + class GlobalRole: """Base class for model-level roles.""" - model_level_permissions: set[str] = set() + model_level_permissions: ClassVar[set[str]] = set()
201-210: LGTM! AuthorizedUser role provides appropriate default permissions.The AuthorizedUser role grants model-level permissions for creating projects and global resources (processing services, taxa) that aren't tied to a specific project. This aligns with the PR objectives to allow authenticated users to create projects.
The permission set is appropriate:
create_project: allows users to create new projectscreate_processingservice,register_pipelines_processingservice: manage global ML servicescreate_taxon,assign_tags_taxon: manage global taxonomyOptional: Consider annotating with
ClassVar(flagged by Ruff at lines 204-210):+from typing import ClassVar + class AuthorizedUser(GlobalRole): """A role that grants project create permission to all authorized users.""" - model_level_permissions = { + model_level_permissions: ClassVar[set[str]] = { "create_project", "create_processingservice", "register_pipelines_processingservice", "create_taxon", "assign_tags_taxon", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ami/main/apps.py(1 hunks)ami/main/tests.py(9 hunks)ami/users/roles.py(6 hunks)ami/users/signals.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T17:19:33.252Z
Learnt from: mihow
Repo: RolnickLab/antenna PR: 1010
File: ami/users/signals.py:78-81
Timestamp: 2025-10-31T17:19:33.252Z
Learning: In the Antenna codebase (ami/users/), the AuthorizedUser group is assigned by default to newly created users but is intentionally removable by administrators. It should not be automatically re-enforced via signals after removal, as it's meant to provide flexible control over which users can create projects.
Applied to files:
ami/users/roles.pyami/users/signals.py
🧬 Code graph analysis (4)
ami/main/apps.py (2)
ami/users/signals.py (2)
create_global_roles(32-48)create_project_based_roles(15-29)ami/tests/fixtures/signals.py (1)
initialize_demo_project(12-19)
ami/main/tests.py (5)
ami/users/roles.py (8)
AuthorizedUser(201-210)BasicMember(127-133)Identifier(140-145)ProjectManager(162-198)assign_user(26-34)assign_user(66-73)get_group_name(19-23)get_group_name(61-63)ami/main/models.py (18)
ProjectManager(173-207)save(319-322)save(920-933)save(1155-1158)save(1498-1505)save(1597-1600)save(1865-1868)save(2146-2167)save(2383-2389)save(2562-2567)save(2850-2867)save(3487-3490)Project(225-485)Permissions(349-418)name(1023-1024)delete(2169-2200)Taxon(3243-3490)create(176-194)ami/ml/views.py (2)
status(176-182)create(159-173)ami/tests/fixtures/main.py (1)
setup_test_project(114-131)ami/ml/models/processing_service.py (1)
create(29-32)
ami/users/roles.py (1)
ami/main/models.py (3)
Project(225-485)Permissions(349-418)name(1023-1024)
ami/users/signals.py (2)
ami/main/models.py (2)
Project(225-485)name(1023-1024)ami/users/roles.py (9)
AuthorizedUser(201-210)GlobalRole(55-124)Role(12-52)create_roles_for_project(213-238)get_group_name(19-23)get_group_name(61-63)sync_group_permissions(96-124)assign_user(26-34)assign_user(66-73)
🪛 Ruff (0.14.3)
ami/main/tests.py
1289-1289: Possible hardcoded password assigned to argument: "password"
(S106)
ami/users/roles.py
15-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
103-103: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
204-210: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/users/signals.py
15-15: Unused function argument: sender
(ARG001)
15-15: Unused function argument: kwargs
(ARG001)
32-32: Unused function argument: sender
(ARG001)
32-32: Unused function argument: kwargs
(ARG001)
44-44: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
47-47: Do not catch blind exception: Exception
(BLE001)
97-97: Unused function argument: sender
(ARG001)
97-97: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (10)
ami/main/apps.py (1)
13-17: LGTM! Signal connection order is appropriate.The signal connection sequence is logical:
initialize_demo_projectcreates the project first, thencreate_project_based_rolesassigns per-project permissions, followed bycreate_global_rolesfor model-level permissions. This ensures dependencies are satisfied in the correct order.ami/users/signals.py (3)
15-29: LGTM! Proper error handling for per-project role creation.The function correctly iterates over all projects and creates roles for each, with appropriate error handling at both the outer and inner levels. Individual project failures are logged but don't interrupt role creation for other projects.
Note: Static analysis flags
senderandkwargsas unused (lines 15), but these parameters are required by Django's signal protocol and cannot be removed.
32-48: LGTM! Clean design for global role synchronization.The function uses
GlobalRole.__subclasses__()to automatically discover all global roles and synchronize their permissions. Thesync_group_permissionsmethod ensures permissions stay up-to-date as role definitions evolve.Note: Static analysis flags
sender,kwargs(line 32) andcreated(line 44) as unused. The first two are required by Django's signal protocol. Thecreatedvariable could be prefixed with_to indicate it's intentionally unused, but this is a minor style issue.
96-100: LGTM! Correctly assigns default group to new users.The signal correctly assigns the AuthorizedUser group only to newly created users. Based on learnings, this group is intentionally removable by administrators and should not be re-enforced after removal.
Note: Static analysis flags
senderandkwargsas unused (line 97), but these are required by Django's signal protocol.ami/main/tests.py (3)
1262-1267: LGTM! Test correctly reflects new permission model.The test method has been appropriately renamed and updated to expect
HTTP_201_CREATEDinstead ofHTTP_403_FORBIDDEN, reflecting that regular users can now create projects through the AuthorizedUser global role.
1276-1390: LGTM! Comprehensive role-based permission testing.This test class provides excellent coverage of the permission system, verifying that:
- Collection-level permissions (create) appear in list responses
- Object-level permissions (update, delete) appear in detail/item responses
- Different roles have appropriate permission sets
- Superuser, owner, project manager, basic member, identifier, and regular users each have correct permissions
The helper methods
_get_project_object,_get_project_list, and_assert_permissions_match_apiprovide a clear testing structure.
3912-3942: LGTM! Comprehensive test for automatic group assignment.This test class verifies that:
- New users are automatically added to the AuthorizedUser group
- The group has all permissions defined in
AuthorizedUser.model_level_permissions- Users inherit these permissions through group membership
The test correctly uses the post_save signal behavior and validates both group membership and effective permissions.
ami/users/roles.py (3)
12-15: LGTM! Clear distinction between object-level and model-level roles.The updated docstring and renamed attribute
object_level_permissions(line 15) clearly indicate that Role is for project-scoped permissions, distinguishing it from the new GlobalRole class for model-level permissions.
127-198: LGTM! Consistent use ofobject_level_permissionsacross role subclasses.All role subclasses (BasicMember, Researcher, Identifier, MLDataManager, ProjectManager) correctly use
object_level_permissionsand compose permissions using set union operators. This maintains a clear hierarchy where higher-level roles inherit lower-level permissions.
213-238: LGTM! Updated to useobject_level_permissionsattribute.The function correctly references
role_class.object_level_permissions(lines 219, 230), aligning with the refactored Role API. The permission clearing and re-assignment logic ensures groups stay synchronized with role definitions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ami/users/roles.py (2)
15-15: Annotate mutable class attribute withClassVar.The
object_level_permissionsclass attribute should be annotated withtyping.ClassVar[set[str]]to indicate it's a class-level attribute that shouldn't be overridden by instances.Apply this diff:
+from typing import ClassVar + class Role: """Base class for project-based object level roles.""" - object_level_permissions = {Project.Permissions.VIEW_PROJECT} + object_level_permissions: ClassVar[set[str]] = {Project.Permissions.VIEW_PROJECT}Note: Apply the same pattern to all role subclasses and the
GlobalRole.model_level_permissionsattribute as flagged by the static analysis tool.
103-103: Prefix unused variable with underscore.The
createdvariable is unpacked but never used.Apply this diff:
- group, created = Group.objects.get_or_create(name=cls.get_group_name()) + group, _ = Group.objects.get_or_create(name=cls.get_group_name())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ami/users/roles.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T17:19:33.252Z
Learnt from: mihow
Repo: RolnickLab/antenna PR: 1010
File: ami/users/signals.py:78-81
Timestamp: 2025-10-31T17:19:33.252Z
Learning: In the Antenna codebase (ami/users/), the AuthorizedUser group is assigned by default to newly created users but is intentionally removable by administrators. It should not be automatically re-enforced via signals after removal, as it's meant to provide flexible control over which users can create projects.
Applied to files:
ami/users/roles.py
🧬 Code graph analysis (1)
ami/users/roles.py (1)
ami/main/models.py (3)
Project(225-485)Permissions(349-418)name(1023-1024)
🪛 Ruff (0.14.3)
ami/users/roles.py
15-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
103-103: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
204-210: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
ami/users/roles.py (3)
127-198: LGTM! Clean refactoring of role subclasses.The migration from
permissionstoobject_level_permissionsis consistent across all role subclasses, and the permission composition using set unions is clear and maintainable.
201-210: LGTM! AuthorizedUser role aligns with PR objectives.The
AuthorizedUserglobal role correctly implements the requirement to grantcreate_projectand related model-level permissions to all authorized users. The permission set matches the requested capabilities from the PR objectives.
213-238: LGTM! Function correctly updated for new permission structure.The
create_roles_for_projectfunction properly usesobject_level_permissionsfrom each role class and correctly resets both group-level and object-level permissions to ensure they stay synchronized with the role definitions.
| def assign_model_level_permissions(cls, group): | ||
| from django.contrib.contenttypes.models import ContentType | ||
|
|
||
| ct = ContentType.objects.get_for_model(Project) | ||
| for perm_codename in cls.model_level_permissions: | ||
| perm_codename = f"{perm_codename}" | ||
| perm, _ = Permission.objects.get_or_create( | ||
| codename=perm_codename, | ||
| content_type=ct, | ||
| defaults={"name": f"Can {perm_codename.replace('_', ' ')} globally"}, | ||
| ) | ||
| logger.info(f"Assigning model-level permission {perm_codename} to group {group.name}") | ||
| group.permissions.add(perm) |
There was a problem hiding this comment.
Address hardcoded content type and code redundancies.
This method has several issues:
- Hardcoded
Projectcontent type limitsGlobalRoleto only Project permissions, preventing reuse for other models' global permissions. - Redundant import of
ContentTypeat line 82 (already imported at line 4). - Redundant f-string at line 86:
perm_codename = f"{perm_codename}"does nothing.
For now, if all model-level permissions are intentionally tied to the Project content type, add a comment explaining this design choice. For future extensibility, consider accepting a model class parameter or storing the content type as a class attribute.
Apply this diff to remove redundancies:
@classmethod
def assign_model_level_permissions(cls, group):
- from django.contrib.contenttypes.models import ContentType
-
+ # All model-level permissions are registered under the Project content type
ct = ContentType.objects.get_for_model(Project)
for perm_codename in cls.model_level_permissions:
- perm_codename = f"{perm_codename}"
perm, _ = Permission.objects.get_or_create(
codename=perm_codename,
content_type=ct,
defaults={"name": f"Can {perm_codename.replace('_', ' ')} globally"},
)
logger.info(f"Assigning model-level permission {perm_codename} to group {group.name}")
group.permissions.add(perm)Based on learnings
🤖 Prompt for AI Agents
In ami/users/roles.py around lines 81 to 93, remove the redundant local import
of ContentType (it's already imported at top) and delete the no-op f-string
assignment perm_codename = f"{perm_codename}"; keep using perm_codename
directly. If model-level permissions are intentionally tied to Project, add a
one-line comment above ct = ContentType... explaining that design decision;
otherwise refactor the method signature to accept a model class or make the
content type a class attribute and use that instead so GlobalRole is not
hardcoded to Project. Also ensure logging and Permission.get_or_create remain
unchanged.
…rriding existing permission names and instead rely on model-level permission names defined in the project model's Meta.permissions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ami/users/roles.py (1)
89-96: Remove redundant import and document the Project content type restriction.Two issues remain from previous review:
- Redundant import:
ContentTypeis imported locally at line 90 but is already imported at the module level (line 4).- Hardcoded Project content type: The method is restricted to Project permissions, which limits
GlobalRoleextensibility. If this is intentional for the current scope, add a comment explaining the design choice.Apply this diff:
@classmethod def assign_model_level_permissions(cls, group): - from django.contrib.contenttypes.models import ContentType - + # All model-level permissions are currently registered under the Project content type ct = ContentType.objects.get_for_model(Project) for perm_codename in cls.model_level_permissions: perm = cls._get_or_update_permission(perm_codename, ct) group.permissions.add(perm) logger.info(f"Assigned model-level permission {perm_codename} to group {group.name}")Based on learnings
🧹 Nitpick comments (2)
ami/users/roles.py (2)
15-15: Consider annotating mutable class attributes withClassVar.Class attributes that hold mutable collections should be annotated with
typing.ClassVarto distinguish them from instance attributes and improve type safety.Apply this diff to add the annotation:
+from typing import ClassVar + class Role: """Base class for project-based object level roles.""" - object_level_permissions = {Project.Permissions.VIEW_PROJECT} + object_level_permissions: ClassVar[set[str]] = {Project.Permissions.VIEW_PROJECT}Note: This same pattern should be applied to
GlobalRole.model_level_permissions(line 58) and role subclass attributes where applicable.
99-124: Use underscore prefix for unused variable.At line 106, the
createdvariable is unpacked but never used. Prefix it with an underscore to follow Python conventions for dummy variables.Apply this diff:
- group, created = Group.objects.get_or_create(name=cls.get_group_name()) + group, _created = Group.objects.get_or_create(name=cls.get_group_name())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ami/users/roles.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T17:19:33.252Z
Learnt from: mihow
Repo: RolnickLab/antenna PR: 1010
File: ami/users/signals.py:78-81
Timestamp: 2025-10-31T17:19:33.252Z
Learning: In the Antenna codebase (ami/users/), the AuthorizedUser group is assigned by default to newly created users but is intentionally removable by administrators. It should not be automatically re-enforced via signals after removal, as it's meant to provide flexible control over which users can create projects.
Applied to files:
ami/users/roles.py
🧬 Code graph analysis (1)
ami/users/roles.py (1)
ami/main/models.py (3)
Project(225-485)Permissions(349-418)name(1023-1024)
🪛 Ruff (0.14.3)
ami/users/roles.py
15-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
106-106: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
203-209: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
ami/users/roles.py (6)
55-64: LGTM!The
GlobalRoleclass andget_group_namemethod are well-structured. The use of the class name as the group name provides a clear, predictable naming convention for model-level permission groups.
65-78: LGTM!The
assign_userandunassign_usermethods correctly manage group membership. The pattern of ensuring permissions are synchronized when assigning users is good practice.
126-159: LGTM!The role classes correctly use
object_level_permissionsand compose permissions through set unions. The permission hierarchy is clear and maintainable.
161-198: LGTM!The
ProjectManagerrole appropriately composes permissions from other roles and adds project management-specific permissions. The comprehensive permission set aligns with the role's responsibilities.
200-210: LGTM!The
AuthorizedUserglobal role correctly implements model-level permissions for authenticated users. The permission set aligns with the PR objectives and includes the permissions mentioned in the call notes (create project, create processing service, register pipelines, create taxon, assign tags to taxon).
212-237: LGTM!The
create_roles_for_projectfunction correctly usesobject_level_permissionsand properly manages permission synchronization by clearing and reassigning permissions on each call.
Summary
This update extends the Antenna permissions framework to support both object-level and model-level permissions.
Previously, permissions were checked only at the object level, using the related project of the object. However, certain models (e.g., those with many-to-many relationships to projects or without a direct project relation) require model-level permission checks. This enhancement introduces a hybrid system that gracefully falls back to Django’s model-level permissions when a model is not directly tied to a specific project.
List of Changes
Added model-level permission checks
The framework now detects whether an object is not tied to a specific project (or is linked to multiple projects) and applies model-level permissions.
Returned model-level permissions to the frontend
Exposed user permissions through the
user_permissionsfield in API responses:Collection level: exposes create permission.
Object level: exposes update and delete permissions and other custom actions permissions.
Enabled all signed-in users to create projects
Introduced a global role (
AuthorizedUser) that grants model-level create_project permission to every signed-in user.Added a signal to automatically assign the
AuthorizedUserrole to every newly registered user.Related Issues
#1006
Detailed Description
Previously, the Antenna permission framework relied solely on object-level checks using project-based permissions . This approach worked well for most models but did not cover cases where models had many-to-many relationships to projects (such as
ProcessingServiceorPipelinemodels) or did not belong to a specific project, like theProjectmodel itself when creating new instances. Only superusers were allowed to manage these models that had no direct project association.With this update, model-level permissions are introduced , enabling a unified and flexible permission system that supports object-level permissions for project-scoped models and model-level permissions for global or multi-project entities. As part of this change, a new global role
AuthenticatedUsershas been introduced and is automatically assigned to every signed-in user. This role grants the create_project model-level permission, meaning all authenticated users can now create new projects without requiring superuser privileges or membership in any existing project.How to Test the Changes
Screenshots
Deployment Notes
Apply migration 0079_assign_authorized_user_group.py to assign all existing users to the AuthenticatedUsers group, enabling them to create projects.
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests
Chores