COP-496-permissions-drf-permission-check#384
Conversation
gabrieleandro
requested review from
Hercilio1,
nossila and
vitorguima
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a PermissionsActionMixin providing a detail-level GET ///permissions endpoint, a normalize_permission utility, integrates the mixin into UsersViewSet with a new Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant View as UsersViewSet
participant Mixin as PermissionsActionMixin
participant Util as normalize_permission
participant DB as DjangoORM
participant Resp as Response
Client->>View: GET /users/<pk>/permissions?perm=perm1&perm=perm2
View->>Mixin: permissions(request, *args, **kwargs)
Mixin->>Mixin: instance = get_object()
Mixin->>Mixin: perms = request.query_params.getlist(permission_query_param)
loop normalize perms
Mixin->>Util: normalize_permission(perm, instance.__class__)
Util-->>Mixin: normalized_perm
end
alt explicit perms provided
loop for each normalized_perm
Mixin->>Mixin: user.has_perm(normalized_perm, instance)
end
Mixin-->>Resp: {"permissions": {...}}
Resp-->>Client: 200 OK
else no perms provided
Mixin->>DB: get_model_permissions_queryset(instance)
DB-->>Mixin: Permission objects
Mixin->>Mixin: build keys and check user.has_perm per permission
Mixin-->>Resp: {"permissions": {...}}
Resp-->>Client: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![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
🤖 Fix all issues with AI agents
In `@baseapp_auth/rest_framework/users/mixins.py`:
- Around line 34-42: The current try/except around building perms from raw_perms
using normalize_permission is catching a bare Exception; change it to catch the
specific errors normalize_permission can raise (e.g., ValueError, TypeError) and
re-raise the ValidationError with exception chaining so the original error is
preserved (use "raise ValidationError({...}) from e"). Update the except block
that wraps the list comprehension for normalize_permission to catch those
specific exceptions (replace "except Exception" with "except (ValueError,
TypeError) as e") and raise ValidationError(... ) from e.
In `@baseapp_auth/rest_framework/users/views.py`:
- Around line 131-139: The current try/except around the comprehension that
calls normalize_permission(raw_perms, user.__class__) catches a bare Exception
and re-raises a ValidationError without chaining; change the block to catch only
the expected exceptions (e.g., ValueError, TypeError) or capture the exception
as err and re-raise the ValidationError using exception chaining (raise
ValidationError({"perm": "Invalid permission format."}) from err) so the
original error context from normalize_permission and the raw_perms processing is
preserved for debugging.
In `@baseapp_auth/utils/normalize_permission.py`:
- Line 22: The file ends without a trailing newline causing flake8 W292; update
the module containing the return statement that builds the permission string
(the line returning f"{opts.app_label}.{codename}" in normalize_permission.py)
by ensuring the file ends with a single newline character — i.e., add a newline
at EOF and save the file.
🧹 Nitpick comments (2)
baseapp_auth/rest_framework/users/mixins.py (2)
44-48: Redundant validation check.The
normalize_permissionfunction always returns a string containing a dot (either passed through from input or constructed as{app_label}.{codename}). This validation will only fail ifnormalize_permissionhas a bug. Consider removing this check or moving it intonormalize_permissionas a post-condition assertion.
27-28: Consider sorting permission keys for consistent API responses.The
perm_keysset (line 62-65) produces non-deterministic ordering. For API consistency and easier client-side caching/comparison, consider sorting the results similar to howviews.pyusessorted(perms)on line 150.🔧 Proposed fix
permissions_map = { perm: user.has_perm(perm, instance) - for perm in perm_keys + for perm in sorted(perm_keys) }Also applies to: 60-70
There was a problem hiding this comment.
Pull request overview
This PR refactors the user-permission endpoints to better support DRF-style permission checks and introduces shared utilities for normalizing permission codenames, including an instance-based permissions mixin.
Changes:
- Added
normalize_permissionutility to normalize various permission string formats intoapp_label.codename. - Extended
UsersViewSetwith a newpermissions_meaction and a reusablePermissionsActionMixinthat exposes an instance-levelpermissionsendpoint. - Updated integration tests for the self-permissions endpoint and removed the previous tests for the nested user-permissions list endpoint.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
baseapp_auth/utils/normalize_permission.py |
Introduces a helper to canonicalize permission identifiers into app_label.codename, used by the new endpoints. |
baseapp_auth/rest_framework/users/views.py |
Adds permissions_me action using normalize_permission, wires in PermissionsActionMixin, and adjusts DRF imports for validation. |
baseapp_auth/rest_framework/users/mixins.py |
Adds PermissionsActionMixin providing a generic instance-based permissions action that checks object permissions via user.has_perm(perm, instance). |
baseapp_auth/tests/integration/test_users.py |
Points tests to the new users-permissions-me route and response shape, and removes the previous TestUserPermissionList coverage for the nested PermissionsViewSet. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -480,73 +480,11 @@ def test_user_can_check_their_permission(self, user_client): | |||
| codename="test_unique_perm", name="Test", content_type=admin_content_type | |||
| ) | |||
| user_client.user.user_permissions.add(perm) | |||
| r = user_client.post(self.reverse(), {"perm": "admin.test_unique_perm"}) | |||
| r = user_client.get(self.reverse(), {"perm": "admin.test_unique_perm"}) | |||
| h.responseOk(r) | |||
| assert r.data["has_perm"] | |||
| assert r.data["permissions"]["admin.test_unique_perm"] is True | |||
|
|
|||
| def test_user_get_false_without_permission(self, user_client): | |||
| r = user_client.post(self.reverse(), {"perm": "admin.test_unique_perm"}) | |||
| r = user_client.get(self.reverse(), {"perm": "admin.test_unique_perm"}) | |||
| h.responseOk(r) | |||
| assert not r.data["has_perm"] | |||
|
|
|||
|
|
|||
| class TestUserPermissionList(ApiMixin): | |||
| view_name = "user-permissions-list" | |||
|
|
|||
| def test_guest_cannot_get_user_permissions(self, client): | |||
| content_type = ContentType.objects.all().first() | |||
| perm = Permission.objects.filter(content_type_id=content_type).first() | |||
| user = UserFactory() | |||
| user.user_permissions.add(perm) | |||
| r = client.get(self.reverse(kwargs={"user_pk": user.pk})) | |||
| h.responseUnauthorized(r) | |||
|
|
|||
| def test_user_without_perm_cannot_get_user_permissions(self, user_client): | |||
| content_type = ContentType.objects.all().first() | |||
| perm = Permission.objects.filter(content_type_id=content_type).first() | |||
| user = UserFactory() | |||
| user.user_permissions.add(perm) | |||
| r = user_client.get(self.reverse(kwargs={"user_pk": user.pk})) | |||
| h.responseBadRequest(r) | |||
| assert "You do not have permission to perform this action." == r.data["detail"] | |||
|
|
|||
| def test_user_with_perm_can_get_user_permissions(self, user_client): | |||
| content_type = ContentType.objects.all().first() | |||
| perm = Permission.objects.filter(content_type_id=content_type).first() | |||
| user = UserFactory() | |||
| user.user_permissions.add(perm) | |||
| p = Permission.objects.get(codename="change_user") | |||
| p.content_type.app_label = "users" | |||
| p.content_type.save() | |||
| user_client.user.user_permissions.add(p) | |||
| user_client.user.refresh_from_db() | |||
| r = user_client.get(self.reverse(kwargs={"user_pk": user.pk})) | |||
| h.responseOk(r) | |||
|
|
|||
| def test_user_with_perm_can_up_user_permissions(self, user_client): | |||
| content_type = ContentType.objects.all().first() | |||
| perm = Permission.objects.filter(content_type_id=content_type).first() | |||
| user = UserFactory() | |||
| user.user_permissions.add(perm) | |||
| p = Permission.objects.get(codename="change_user") | |||
| p.content_type.app_label = "users" | |||
| p.content_type.save() | |||
| user_client.user.user_permissions.add(p) | |||
| r = user_client.post( | |||
| self.reverse(kwargs={"user_pk": user.pk}), data={"codename": "delete_user"} | |||
| ) | |||
| h.responseCreated(r) | |||
| assert user.user_permissions.count() == 2 | |||
|
|
|||
| def test_user_with_perm_can_set_user_permissions(self, user_client): | |||
| user = UserFactory() | |||
| p = Permission.objects.get(codename="change_user") | |||
| p.content_type.app_label = "users" | |||
| p.content_type.save() | |||
| user_client.user.user_permissions.add(p) | |||
| r = user_client.post( | |||
| self.reverse(kwargs={"user_pk": user.pk}), | |||
| data={"permissions": ["change_user", "delete_user"]}, | |||
| ) | |||
| h.responseCreated(r) | |||
| assert user.user_permissions.count() == 2 | |||
| assert r.data["permissions"]["admin.test_unique_perm"] is not True | |||
There was a problem hiding this comment.
The previous TestUserPermissionList tests for the nested PermissionsViewSet (registered as basename="user-permissions" in rest_framework/routers/account.py) have been removed from this file, but the viewset and router registration are still present. This drops integration coverage for listing and mutating another user's permissions; if that API remains supported, consider retaining or relocating equivalent tests so regressions in that endpoint are still caught.
|
@gabrieleandro I see you've added in ur PR description a video showing the new admin permissions interface. But I don't think it's related to this PR is it? I think this more related to how permissions are now being exposed through and endpoint... |
|
@gabrieleandro add breaking change url path for permissions |
…missions-drf-permission-check
3 participants
Demo:
https://www.loom.com/share/e55e20c69cfc443a9010e91ed7939057
Summary by CodeRabbit
New Features
Utilities
Tests