feat: Add model-level permissions framework (rebase test - no not merge)#1110
feat: Add model-level permissions framework (rebase test - no not merge)#1110
Conversation
…he response `user_permissions` field
… ProcessingService models
…t_permissions parameter order
…TestCase to remove duplication
…ns for creating processing services, registering pipelines, creating taxa, and assigning tags
…rriding existing permission names and instead rely on model-level permission names defined in the project model's Meta.permissions.
Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
📝 WalkthroughWalkthroughThis pull request refactors the permission system to support model-level and object-level permissions separately, introducing a PermissionsMixin to centralize permission logic and enabling all authorized users to create projects through global roles and permissions groups. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API as API View
participant Model as Model Instance
participant Mixin as PermissionsMixin
participant Guardian as Guardian/Django Perms
participant Signal as Signal Handler
User->>API: POST /projects/
API->>Model: create instance + check_permission(user, 'create')
Model->>Mixin: check_permission(user, 'create')
Mixin->>Mixin: action=='create' → check_model_level_permission
Mixin->>Guardian: has_perm(user, 'main.create_project')
Guardian-->>Mixin: permission result
Mixin-->>Model: boolean result
Model-->>API: permission granted/denied
alt Permission Granted
API->>Model: save(owner=user)
Note over Model: Instance saved to DB
Model->>Signal: post_save signal triggered
Signal->>Signal: assign_authorized_user_group(user)
Note over Signal: User added to AuthorizedUser group
else Permission Denied
API-->>User: HTTP 403 PermissionDenied
end
API-->>User: HTTP 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mihow
changed the title
Code reviewFound 1 issue:
In
This will return wrong permission names for any multi-word custom permission. antenna/ami/base/permissions.py Lines 280 to 288 in cd5c17c 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ami/main/models.py (2)
333-364:⚠️ Potential issue | 🟠 MajorProject custom actions are no longer routed through the new mixin hook.
PermissionsMixinnow callscheck_custom_object_level_permission, butProjectstill definescheck_custom_permission, so thechartsaction falls back to the default codename check and will likely be denied for non‑superusers. Rename the override to keep the custom logic active.🛠️ Suggested fix
- def check_custom_permission(self, user, action: str) -> bool: + def check_custom_object_level_permission(self, user, action: str) -> bool: """ Check custom permissions for actions like 'charts'. Charts is treated as a read-only operation, so it follows the same permission logic as 'retrieve'. """ from ami.users.roles import BasicMember if action == "charts": # Same permission logic as retrieve action if self.draft: # Allow view permission for members and owners of draft projects return BasicMember.has_role(user, self) or user == self.owner or user.is_superuser return True # Fall back to default permission checking for other actions - return super().check_custom_permission(user, action) + return super().check_custom_object_level_permission(user, action)
1984-2006:⚠️ Potential issue | 🟠 MajorFall back to base custom permission checks for non‑star actions.
The override returnsNonefor other actions, which denies any other custom permission even if the user has it.🛠️ Suggested fix
def check_custom_object_level_permission(self, user, action: str) -> bool: project = self.get_project() if hasattr(self, "get_project") else None if action in ["star", "unstar"]: return user.has_perm(Project.Permissions.STAR_SOURCE_IMAGE, project) + return super().check_custom_object_level_permission(user, action)
🤖 Fix all issues with AI agents
In `@ami/base/permissions.py`:
- Around line 72-76: Guard against user being None and ensure
response_data["user_permissions"] is treated as a set before updating: if user
is None, replace it with an AnonymousUser instance (import from
django.contrib.auth.models.AnonymousUser) before calling
model.get_collection_level_permissions(user=..., project=...), and normalize
response_data.get("user_permissions") by coercing any existing list (or other
iterable) into a set before calling .update(); after merging
collection_level_perms into that set, store the result back as a list in
response_data["user_permissions"]. Use the surrounding identifiers
(response_data, model.get_collection_level_permissions, collection_level_perms,
user) to locate where to apply this change.
In `@ami/main/migrations/0079_assign_authorized_user_group.py`:
- Around line 57-65: In the backwards function remove the dead fetches: delete
the unused Permission = apps.get_model("auth", "Permission") line and stop
assigning project_ct from ContentType.objects.get; instead perform an existence
check (e.g. ContentType.objects.filter(app_label="main",
model="project").exists()) or call ContentType.objects.get without assigning to
a variable if you only need to test presence, and remove the
logger.info("Fetched ContentType for Project model.") line; keep the ContentType
lookup/exception handling logic but avoid storing unused variables (refer to
Permission and project_ct and the ContentType.objects.get call to locate edits).
In `@ami/ml/views.py`:
- Line 13: The status action currently fetches the instance via
ProcessingService.objects.get(pk=pk) and bypasses object-level checks; change it
to use self.get_object() inside the status view method so ObjectPermission (and
the model's check_permission()) runs just like register_pipelines does; locate
the status method in the view, replace the direct
ProcessingService.objects.get(...) usage with self.get_object() and ensure any
subsequent logic uses that returned instance.
🧹 Nitpick comments (3)
ami/main/models.py (1)
366-375: Silence the unusedprojectparameter.
Rename to_project(or remove if the interface allows) to avoid AR003 without changing behavior.♻️ Suggested tweak
- def get_collection_level_permissions( - cls, user: AbstractUser | AnonymousUser, project: "Project | None" = None - ) -> list[str]: + def get_collection_level_permissions( + cls, user: AbstractUser | AnonymousUser, _project: "Project | None" = None + ) -> list[str]:ami/users/signals.py (1)
44-44: Minor: Prefix unused variable with underscore.The
createdvariable fromget_or_createis never used.Proposed fix
- group, created = Group.objects.get_or_create(name=role_cls.get_group_name()) + group, _created = Group.objects.get_or_create(name=role_cls.get_group_name())ami/main/migrations/0079_assign_authorized_user_group.py (1)
44-48: Consider using bulk operations for better performance.The current implementation iterates over all users and calls
user.groups.add(group)individually, which creates N database queries. For large user bases, using bulk operations would be more efficient.Proposed improvement using through model
- for idx, user in enumerate(User.objects.all().iterator(), start=1): - user.groups.add(group) - if idx % 100 == 0: - logger.info(f"Processed {idx}/{total_users} users...") + # Use bulk_create on the through model for better performance + UserGroups = User.groups.through + existing_memberships = set( + UserGroups.objects.filter(group=group).values_list('user_id', flat=True) + ) + + new_memberships = [ + UserGroups(user_id=user_id, group_id=group.id) + for user_id in User.objects.values_list('id', flat=True).iterator() + if user_id not in existing_memberships + ] + + UserGroups.objects.bulk_create(new_memberships, ignore_conflicts=True) + logger.info(f"Assigned 'AuthorizedUser' group to {len(new_memberships)} users.")
| logger.info(f"add_collection_level_permissions model {model.__name__}, {type(model)} ") | ||
| permissions = response_data.get("user_permissions", set()) | ||
| if user and user.is_superuser: | ||
| permissions.add("create") | ||
| if user and project and f"create_{model.__name__.lower()}" in get_perms(user, project): | ||
| permissions.add("create") | ||
| collection_level_perms = model.get_collection_level_permissions(user=user, project=project) | ||
| permissions.update(collection_level_perms) | ||
| response_data["user_permissions"] = list(permissions) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the function and its context
cd / && find . -name "permissions.py" -path "*/ami/base/*" 2>/dev/null | head -5Repository: RolnickLab/antenna
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Check the function signature and context around lines 72-76
cat -n ami/base/permissions.py | sed -n '50,100p'Repository: RolnickLab/antenna
Length of output: 2715
🏁 Script executed:
#!/bin/bash
# Search for call sites of this function
rg -n "add_collection_level_permissions" --type py -C 3Repository: RolnickLab/antenna
Length of output: 2021
🏁 Script executed:
#!/bin/bash
# Search for get_collection_level_permissions to understand its implementation
rg -n "def get_collection_level_permissions" --type py -A 5Repository: RolnickLab/antenna
Length of output: 906
🏁 Script executed:
#!/bin/bash
# Check how user_permissions is set and used elsewhere
rg -n "user_permissions" ami/base/permissions.py -B 2 -A 2Repository: RolnickLab/antenna
Length of output: 1057
🏁 Script executed:
#!/bin/bash
# Find all calls to add_collection_level_permissions
rg -n "add_collection_level_permissions\(" --type py -B 2 -A 2Repository: RolnickLab/antenna
Length of output: 802
🏁 Script executed:
#!/bin/bash
# Check if user can be None in those call contexts
rg -n "self\.request\.user" ami/base/pagination.py -B 5 -A 5Repository: RolnickLab/antenna
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Check the get_collection_level_permissions implementation to see what happens with None
cat -n ami/base/permissions.py | sed -n '291,310p'Repository: RolnickLab/antenna
Length of output: 1134
🏁 Script executed:
#!/bin/bash
# Check if there are any explicit user=None calls anywhere
rg -n "user=None" --type py | grep -i "collection\|permission"Repository: RolnickLab/antenna
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Check if add_collection_level_permissions could be called multiple times on the same response_data
rg -n "add_collection_level_permissions\|add_object_level_permissions" --type py -B 5 -A 5Repository: RolnickLab/antenna
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check what super().get_paginated_response() returns to understand response_data initial state
rg -n "get_paginated_response" ami/base/pagination.py -A 15Repository: RolnickLab/antenna
Length of output: 869
🏁 Script executed:
#!/bin/bash
# Verify if both add_object_level_permissions and add_collection_level_permissions are called on the same response_data
rg -n "add_object_level_permissions\|add_collection_level_permissions" --type py -C 5 | head -50Repository: RolnickLab/antenna
Length of output: 44
Guard against user=None and normalize user_permissions to a set.
The function signature allows user: User | None, but model.get_collection_level_permissions() expects user: AbstractUser | AnonymousUser and will raise AttributeError if None is passed. Additionally, line 73 assumes response_data.get("user_permissions", set()) returns a set, but since line 76 stores it as a list, a subsequent call on the same response_data will fail when calling .update() on a list.
🛠️ Suggested fix
- permissions = response_data.get("user_permissions", set())
- collection_level_perms = model.get_collection_level_permissions(user=user, project=project)
+ permissions = set(response_data.get("user_permissions", []))
+ if user is None:
+ user = AnonymousUser()
+ collection_level_perms = model.get_collection_level_permissions(user=user, project=project)
permissions.update(collection_level_perms)🤖 Prompt for AI Agents
In `@ami/base/permissions.py` around lines 72 - 76, Guard against user being None
and ensure response_data["user_permissions"] is treated as a set before
updating: if user is None, replace it with an AnonymousUser instance (import
from django.contrib.auth.models.AnonymousUser) before calling
model.get_collection_level_permissions(user=..., project=...), and normalize
response_data.get("user_permissions") by coercing any existing list (or other
iterable) into a set before calling .update(); after merging
collection_level_perms into that set, store the result back as a list in
response_data["user_permissions"]. Use the surrounding identifiers
(response_data, model.get_collection_level_permissions, collection_level_perms,
user) to locate where to apply this change.
| Permission = apps.get_model("auth", "Permission") | ||
| ContentType = apps.get_model("contenttypes", "ContentType") | ||
|
|
||
| try: | ||
| project_ct = ContentType.objects.get(app_label="main", model="project") | ||
| logger.info("Fetched ContentType for Project model.") | ||
| except ContentType.DoesNotExist: | ||
| logger.warning("ContentType for Project model not found. Nothing to reverse.") | ||
| return |
There was a problem hiding this comment.
Remove unused variables in backwards function.
Permission and project_ct are fetched but never used. This is dead code that should be cleaned up.
Proposed fix
def backwards(apps, schema_editor):
logger.info("Reversing migration: Removing 'AuthorizedUser' role from users...")
User = apps.get_model("users", "User")
Group = apps.get_model("auth", "Group")
- Permission = apps.get_model("auth", "Permission")
- ContentType = apps.get_model("contenttypes", "ContentType")
-
- try:
- project_ct = ContentType.objects.get(app_label="main", model="project")
- logger.info("Fetched ContentType for Project model.")
- except ContentType.DoesNotExist:
- logger.warning("ContentType for Project model not found. Nothing to reverse.")
- return
try:
group = Group.objects.get(name="AuthorizedUser")📝 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.
| Permission = apps.get_model("auth", "Permission") | |
| ContentType = apps.get_model("contenttypes", "ContentType") | |
| try: | |
| project_ct = ContentType.objects.get(app_label="main", model="project") | |
| logger.info("Fetched ContentType for Project model.") | |
| except ContentType.DoesNotExist: | |
| logger.warning("ContentType for Project model not found. Nothing to reverse.") | |
| return | |
| User = apps.get_model("users", "User") | |
| Group = apps.get_model("auth", "Group") | |
| try: | |
| group = Group.objects.get(name="AuthorizedUser") |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 57-57: Local variable Permission is assigned to but never used
Remove assignment to unused variable Permission
(F841)
[error] 61-61: Local variable project_ct is assigned to but never used
Remove assignment to unused variable project_ct
(F841)
🤖 Prompt for AI Agents
In `@ami/main/migrations/0079_assign_authorized_user_group.py` around lines 57 -
65, In the backwards function remove the dead fetches: delete the unused
Permission = apps.get_model("auth", "Permission") line and stop assigning
project_ct from ContentType.objects.get; instead perform an existence check
(e.g. ContentType.objects.filter(app_label="main", model="project").exists()) or
call ContentType.objects.get without assigning to a variable if you only need to
test presence, and remove the logger.info("Fetched ContentType for Project
model.") line; keep the ContentType lookup/exception handling logic but avoid
storing unused variables (refer to Permission and project_ct and the
ContentType.objects.get call to locate edits).
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
|
|
||
| from ami.base.permissions import ObjectPermission |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and inspect the ObjectPermission class
echo "=== ObjectPermission class ==="
rg -n "class ObjectPermission" -S -A 10
echo ""
echo "=== ProcessingServiceViewSet class and status action ==="
rg -n "class ProcessingServiceViewSet" -S -A 50 | head -80
echo ""
echo "=== status action in ami/ml/views.py ==="
rg -n "def status" -A 15 ami/ml/views.py
echo ""
echo "=== Permission class assignment in viewset ==="
rg -n "permission_classes" ami/ml/views.py
echo ""
echo "=== ProcessingService model ==="
rg -n "class ProcessingService" -S -A 10Repository: RolnickLab/antenna
Length of output: 13078
Replace direct object fetch with self.get_object() in the status action to enforce object-level permissions.
The status action at line 185 bypasses permission checks by directly querying ProcessingService.objects.get(pk=pk), while the register_pipelines action at line 191 correctly uses self.get_object() to trigger ObjectPermission.has_object_permission(). Since ObjectPermission delegates authorization to the model's check_permission() method, the status action must follow the same pattern.
Suggested fix
- processing_service = ProcessingService.objects.get(pk=pk)
+ processing_service = self.get_object()🤖 Prompt for AI Agents
In `@ami/ml/views.py` at line 13, The status action currently fetches the instance
via ProcessingService.objects.get(pk=pk) and bypasses object-level checks;
change it to use self.get_object() inside the status view method so
ObjectPermission (and the model's check_permission()) runs just like
register_pipelines does; locate the status method in the view, replace the
direct ProcessingService.objects.get(...) usage with self.get_object() and
ensure any subsequent logic uses that returned instance.
High-Level Architecture ReviewThe hybrid permission approach (model-level vs object-level based on Observations to consider:
Overall the approach is sound - it doesn't require major refactoring, uses familiar Django patterns, and has good test coverage. |
2 participants
Summary
This update extends the Antenna permissions framework to support both object-level and model-level permissions.
user_permissionsfield in API responses at both collection and object levelsKey Changes
Backend
GlobalRolebase class for model-level permissionscheck_permission,check_model_level_permission, andcheck_object_level_permissionmethodsProcessingServiceandTaxonmodelsPermissions
create_project- Create new projects globallycreate_processingservice,update_processingservice,delete_processingservice- Manage processing servicesregister_pipelines_processingservice- Register pipelines for processing servicescreate_taxon,update_taxon,delete_taxon- Manage taxa globallyassign_tags_taxon- Assign tags to taxaTest plan
Related Issues
Closes #1006
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.