[feature] Added API endpoint to manage RADIUS groups of users #676#680
[feature] Added API endpoint to manage RADIUS groups of users #676#680nemesifier merged 10 commits intomasterfrom
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds REST API endpoints to manage per-user RADIUS group assignments: list/create at /api/v1/users/user/<user_pk>/radius-group/ and detail (retrieve/update/destroy) at /api/v1/users/user/<user_pk>/radius-group//. Introduces RadiusUserGroupSerializer (limits group choices to the parent user's organizations and injects user info), new views (BaseRadiusUserGroupView, RadiusUserGroupListCreateView, RadiusUserGroupDetailView) with organization-aware queryset/filtering and permission checks, URL registrations, tests for list/detail flows, and documentation updates. Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Server\n(RadiusUserGroupListCreateView)"
participant Auth as "Auth/Permission\nChecks"
participant DB as "Database\n(User, RadiusGroup, RadiusUserGroup)"
Client->>API: GET/POST /users/user/{user_pk}/radius-group/
API->>Auth: verify authentication & permissions
Auth-->>API: allow/deny
alt allowed
API->>DB: fetch parent User & organizations
DB-->>API: parent user + orgs
API->>DB: (GET) query RadiusUserGroup filtered by orgs
DB-->>API: list results
API-->>Client: 200 with list
opt POST flow
Client->>API: POST with group UUID, optional priority
API->>DB: create RadiusUserGroup (validate same-organization)
DB-->>API: created instance
API-->>Client: 201 with created object
end
else denied
API-->>Client: 401/403
end
sequenceDiagram
participant Client
participant API as "API Server\n(RadiusUserGroupDetailView)"
participant Auth as "Auth/Permission\nChecks"
participant DB as "Database\n(RadiusUserGroup)"
Client->>API: GET/PATCH/PUT/DELETE /users/user/{user_pk}/radius-group/{id}/
API->>Auth: verify authentication & permissions
Auth-->>API: allow/deny
alt allowed
API->>DB: fetch RadiusUserGroup by id constrained to parent user
DB-->>API: instance or 404
alt found
opt Update (PATCH/PUT)
API->>DB: validate (ensure group same-organization) and update
DB-->>API: updated instance
API-->>Client: 200 with updated data
end
opt Delete
API->>DB: delete instance
DB-->>API: confirmation
API-->>Client: 204
end
else not found
API-->>Client: 404
end
else denied
API-->>Client: 401/403
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In `@docs/user/rest-api.rst`:
- Around line 1011-1025: Fix the PATCH section description and parameter
optionality: change the summary sentence that currently reads "Partially updates
a RADIUS group identified by its UUID." to "Partially updates a RADIUS user
group assignment." (this is the PATCH section header/first sentence) and update
the parameters table entries for `group` and `priority` so neither is marked as
"required" — document both as optional to reflect PATCH's partial-update
behavior; ensure `group` still notes it expects a UUID and `priority` remains an
integer.
In `@openwisp_radius/tests/test_api/test_api.py`:
- Line 1342: Remove the leftover debug print by deleting the
print(response.data) statement in the test (the debug print in
openwisp_radius/tests/test_api/test_api.py); ensure the test relies on
assertions or proper logging instead of printing response.data to avoid noisy
test output.
🧹 Nitpick comments (1)
openwisp_radius/api/serializers.py (1)
386-390: Misleadingref_nameonGroupSerializer.
ref_name = "radius_user_group_serializer"on the Django authGroupserializer is confusing — it implies this serializer is forRadiusUserGroupwhen it actually serializesdjango.contrib.auth.models.Group. Consider a clearer name like"radius_auth_group"to avoid confusion in the OpenAPI schema.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/user/rest-api.rstopenwisp_radius/api/serializers.pyopenwisp_radius/api/urls.pyopenwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.pytests/openwisp2/sample_radius/api/views.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/openwisp2/sample_radius/api/views.py (1)
openwisp_radius/api/views.py (4)
RadiusGroupDetailView(942-946)RadiusGroupListView(896-904)RadiusUserGroupDetailView(1039-1040)RadiusUserGroupListCreateView(1000-1001)
openwisp_radius/api/serializers.py (3)
openwisp_radius/base/models.py (14)
Meta(312-316)Meta(344-348)Meta(535-539)Meta(618-622)Meta(649-652)Meta(749-754)Meta(785-789)Meta(817-821)Meta(851-855)Meta(939-944)Meta(1173-1177)Meta(1330-1333)Meta(1469-1477)Meta(1648-1651)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (1)
RadiusUserGroup(58-60)
openwisp_radius/api/views.py (2)
openwisp_radius/api/serializers.py (2)
RadiusUserGroupSerializer(368-383)user(716-717)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)
⏰ 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). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
🔇 Additional comments (7)
tests/openwisp2/sample_radius/api/views.py (1)
20-25: New view imports and bindings follow the existing pattern for RadiusGroup views.The direct import approach (without local subclassing) is consistent with how
RadiusGroupListViewandRadiusGroupDetailVieware already handled in this file (lines 125-126). No issues.Also applies to: 127-128
openwisp_radius/api/urls.py (1)
101-110: URL patterns look correct.The nested resource pattern under
users/user/<user_pk>/is appropriate for user-scoped RADIUS group assignments. Using<str:user_pk>accommodates both integer and UUID-based user primary keys.openwisp_radius/tests/test_api/test_api.py (1)
1295-1494: Tests are thorough and cover key scenarios well.Good coverage of authentication, authorization, cross-org restrictions, CRUD operations, and superuser access for both list and detail endpoints.
One optional gap: consider adding a test case for PATCH/PUT on the detail endpoint that attempts to change the
groupto one from a different organization (similar to the "Cannot create RadiusUserGroup with group from other org" subtest intest_radius_user_group_list).openwisp_radius/api/views.py (3)
951-981:BaseRadiusUserGroupViewis well-structured with proper parent and organization filtering.The separation of
get_queryset,get_parent_queryset, andget_organization_querysetis clean and follows theFilterByParentManagedpattern. Theselect_relatedon the base queryset is a good optimization.
1039-1040:organization_fieldcorrectly set for organization-based filtering.Using
"group__organization"properly traverses the relationship fromRadiusUserGroup→RadiusGroup→Organizationfor permission checks on detail operations.
51-55: Imports are correct.
FilterByParentManagedandRadiusUserGroupSerializerare properly imported and used by the new view classes.Also applies to: 76-76
openwisp_radius/api/serializers.py (1)
376-383: PotentialAttributeErrorif parent user does not exist.
view.get_parent_queryset().first()returnsNonewhen the user PK from the URL doesn't exist. In that case,user.usernameon line 380 will raise anAttributeError, resulting in a 500 error instead of a clean 404.While
FilterByParentManagedis designed to validate parent existence at the view level and should prevent this, there is no explicit test case for a completely non-existentuser_pk, making it difficult to confirm the exact behavior. Adding a defensive check here would ensure protection regardless of the parent validation order.Proposed defensive fix
def validate(self, data): view = self.context.get("view") user = view.get_parent_queryset().first() + if user is None: + raise serializers.ValidationError( + {"user": _("User not found.")} + ) if "username" not in data: data["username"] = user.username if "user" not in data: data["user"] = user return super().validate(data)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pandafy
force-pushed
the
issues/676-api-radius-user-group
branch
3 times, most recently
from
e7ee368 to
84b13c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_radius/api/urls.py`:
- Around line 101-110: The URL patterns for radius_user_group_list and
radius_user_group_detail currently use the parameter "<str:user_pk>" which
allows arbitrary strings; change both to use "<uuid:user_pk>" so Django
validates UUIDs at routing and stays consistent with other patterns (e.g., those
using "<uuid:pk>"). Update the two path() calls referencing
radius_user_group_list and radius_user_group_detail to replace "str:user_pk"
with "uuid:user_pk" and run tests to ensure URL reversing and view lookups still
work.
🧹 Nitpick comments (4)
openwisp_radius/api/serializers.py (2)
374-381: Consider guarding against a missing parent user.If
get_parent_queryset().first()returnsNone(e.g., due to a race condition or the parent check not yet having run), thegroupfield's queryset will remain unfiltered at this level. WhileFilterSerializerByOrgManagedand the view's parent check likely prevent this in practice, adding a defensive guard would make the intent clearer and prevent accidental misuse.Proposed fix
def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) view = self.context.get("view") self._user = view.get_parent_queryset().first() if self._user: self.fields["group"].queryset = self.fields["group"].queryset.filter( organization_id__in=self._user.organizations_dict.keys() ) + else: + self.fields["group"].queryset = self.fields["group"].queryset.none()
392-396: Misleadingref_nameonGroupSerializer.
ref_name = "radius_user_group_serializer"onGroupSerializer(which serializesdjango.contrib.auth.models.Group) could easily be confused with the newly addedRadiusUserGroupSerializer. A clearer name like"radius_auth_group_serializer"or"radius_django_group_serializer"would reduce ambiguity in the Swagger schema.openwisp_radius/api/views.py (1)
963-967: Minor: redundantsuper().get_queryset()call in swagger branch.
qsalready holdssuper().get_queryset(). The swagger branch can simply returnqs.Proposed fix
def get_queryset(self): qs = super().get_queryset() if getattr(self, "swagger_fake_view", False): - return super().get_queryset() + return qs return qs.filter(user_id=self.kwargs["user_pk"])openwisp_radius/tests/test_api/test_api.py (1)
1295-1389: Good coverage, but consider adding a multi-org user scenario.The tests thoroughly cover auth, permissions, cross-org restrictions, and superuser flows. However, a scenario where the target user belongs to multiple organizations while the admin manages only one is not tested. This would validate that the list endpoint doesn't leak RadiusUserGroups from organizations the admin doesn't manage — directly tied to the
organization_fieldconcern flagged in the views.Example scenario to add:
target_useris a member of bothorg1andorg2admin_usermanages onlyorg1target_userhas RadiusUserGroups in both org1 and org2- Assert the list endpoint only returns org1's RadiusUserGroups
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/user/rest-api.rstopenwisp_radius/api/serializers.pyopenwisp_radius/api/urls.pyopenwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.pytests/openwisp2/sample_radius/api/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/user/rest-api.rst
🧰 Additional context used
🧬 Code graph analysis (4)
openwisp_radius/tests/test_api/test_api.py (7)
openwisp_radius/api/serializers.py (1)
user(722-723)openwisp_radius/models.py (2)
RadiusGroup(43-46)RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (2)
RadiusGroup(43-45)RadiusUserGroup(58-60)openwisp_radius/api/views.py (2)
get(166-178)get(707-719)openwisp_radius/api/freeradius_views.py (1)
get(495-500)openwisp_radius/tests/__init__.py (1)
_create_radius_usergroup(123-128)openwisp_radius/base/models.py (2)
delete(686-689)delete(1076-1079)
openwisp_radius/api/views.py (3)
openwisp_radius/api/serializers.py (2)
RadiusUserGroupSerializer(368-389)user(722-723)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (1)
RadiusUserGroup(58-60)
openwisp_radius/api/serializers.py (3)
openwisp_radius/api/views.py (5)
Meta(424-427)Meta(870-871)get(166-178)get(707-719)get_parent_queryset(969-971)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (1)
RadiusUserGroup(58-60)
tests/openwisp2/sample_radius/api/views.py (1)
openwisp_radius/api/views.py (4)
RadiusGroupDetailView(942-946)RadiusGroupListView(896-904)RadiusUserGroupDetailView(1041-1042)RadiusUserGroupListCreateView(1002-1003)
⏰ 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). (9)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (4)
openwisp_radius/api/views.py (2)
973-983:get_organization_querysetcorrectly scopes parent users — LGTM.Filters by managed orgs and excludes superusers, with
.distinct()to handle the join. The dynamicapp_labelconstruction for the organization user lookup follows the established openwisp pattern.
952-984: Revert this review:organization_fieldis not used for child queryset filtering inFilterByParentManaged.The original analysis assumes that setting
organization_field = "group__organization"on a view will filter the child RadiusUserGroup queryset by organization. However, examining theFilterByParentManagedmixin from openwisp-users shows thatorganization_fieldis used only for filtering the parent queryset (the User) during theassert_parent_exists()validation. The mixin does not filter the child queryset based on this attribute.Both
RadiusUserGroupListCreateViewandRadiusUserGroupDetailViewinherit the same parent validation logic fromFilterByParentManaged. Movingorganization_fieldto the base class would have no effect on child queryset filtering and would not resolve any multi-tenancy vulnerability.If there is an actual multi-tenancy concern (e.g., admin managing only Org1 can see RadiusUserGroups from Org2 for a user that belongs to multiple organizations), the proper fix would be to filter the child queryset by the group's organization in the
get_queryset()method, not by settingorganization_field.Likely an incorrect or invalid review comment.
tests/openwisp2/sample_radius/api/views.py (1)
20-25: LGTM — consistent with the existing pattern for RadiusGroup views.The direct import and
as_view()binding pattern matches howRadiusGroupListViewandRadiusGroupDetailVieware already handled in this file (lines 21-22, 125-126).Also applies to: 127-128
openwisp_radius/tests/test_api/test_api.py (1)
1391-1493: Detail endpoint tests look solid.Comprehensive coverage for all CRUD operations, authentication, permissions, 404 handling, cross-org isolation, and superuser access.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pandafy
force-pushed
the
issues/676-api-radius-user-group
branch
from
af7d042 to
52da134
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/api/serializers.py`:
- Around line 374-377: The serializer __init__ assumes a nested view and parent
queryset exist and can crash; update __init__ in the serializer to defensively
get view = self.context.get("view") and only call view.get_parent_queryset() if
view is truthy and has that method, assigning self._user = parent_qs.first()
when available; if view is missing or parent queryset is None, fall back to
resolve a user from self.initial_data.get("user_pk") (or
self.context.get("user_pk")) and set self._user accordingly, and otherwise set
self._user = None so initialization never raises when view/user_pk are absent.
In `@openwisp_radius/api/views.py`:
- Around line 1002-1003: RadiusUserGroupListCreateView lacks organization
filtering for ?organization=...; update the class
(RadiusUserGroupListCreateView) to enable filtering by adding
DjangoFilterBackend to filter_backends and exposing organization (e.g., via
filterset_fields = ("organization",) or by assigning a filterset_class like
RadiusUserGroupFilter that includes an organization field), and ensure
DjangoFilterBackend is imported (from django_filters.rest_framework import
DjangoFilterBackend) so the view honors the ?organization query param.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
openwisp_radius/api/serializers.pyopenwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.py
📜 Review details
⏰ 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). (11)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_radius/tests/test_api/test_api.py (6)
openwisp_radius/api/serializers.py (1)
user(724-725)openwisp_radius/models.py (2)
RadiusGroup(43-46)RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (2)
RadiusGroup(43-45)RadiusUserGroup(58-60)openwisp_radius/tests/__init__.py (1)
_create_radius_usergroup(123-128)tests/openwisp2/sample_users/models.py (1)
OrganizationUser(37-39)openwisp_radius/base/models.py (2)
delete(686-689)delete(1076-1079)
openwisp_radius/api/serializers.py (3)
openwisp_radius/base/models.py (14)
Meta(312-316)Meta(344-348)Meta(535-539)Meta(618-622)Meta(649-652)Meta(749-754)Meta(785-789)Meta(817-821)Meta(851-855)Meta(939-944)Meta(1173-1177)Meta(1330-1333)Meta(1469-1477)Meta(1648-1651)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (1)
RadiusUserGroup(58-60)
openwisp_radius/api/views.py (2)
openwisp_radius/api/serializers.py (2)
RadiusUserGroupSerializer(368-391)user(724-725)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)
🔇 Additional comments (1)
openwisp_radius/tests/test_api/test_api.py (1)
1295-1511: Strong authorization and CRUD coverage for the new RadiusUserGroup endpoints.The new tests validate unauthenticated/unauthorized paths, cross-organization boundaries, and superuser behavior well across both list/create and detail flows.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/rest-api.rst`:
- Around line 983-984: Replace the incorrect word "sorting" with "filtering" in
the endpoint description sentence that currently reads "It supports sorting by
organization id and organization slug." — update it to "It supports filtering by
organization id and organization slug." and ensure any nearby uses of "sorting"
in the same endpoint description are likewise changed to "filtering" to
accurately describe query-parameter behavior.
- Around line 1018-1038: The detail endpoint docs currently list only
GET/PATCH/DELETE but the API also supports PUT (full update); add a "PUT"
subsection under the same detail block (near the "GET (detail)", "PATCH", and
"DELETE" headings) that documents full updates for the RADIUS user group
assignment, describing that PUT performs a full replacement of the resource and
listing the same parameters (group UUID and priority integer) and any
required/optional semantics so behavior matches the exposed API. Reference the
"GET (detail)", "PATCH", and "DELETE" sections when inserting the new "PUT"
subsection to keep ordering and formatting consistent.
In `@openwisp_radius/tests/test_api/test_api.py`:
- Around line 1395-1401: The test incorrectly grants org2 admin membership to
target_user instead of admin_user; update the setup so that
OrganizationUser.objects.filter(..., user=target_user,
organization=org2).delete() remains but call
self._create_org_user(user=admin_user, organization=org2, is_admin=True)
(instead of user=target_user) so the subtest matches the comment scenario where
target_user is only a member of org1 and admin_user can manage both org1 and
org2.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/user/rest-api.rstopenwisp_radius/api/serializers.pyopenwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.py
📜 Review details
⏰ 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). (2)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_radius/api/serializers.py (4)
openwisp_radius/api/views.py (6)
Meta(424-427)Meta(870-871)Meta(993-995)get(166-178)get(707-719)get_parent_queryset(969-971)openwisp_radius/base/models.py (13)
Meta(312-316)Meta(344-348)Meta(535-539)Meta(618-622)Meta(649-652)Meta(749-754)Meta(785-789)Meta(817-821)Meta(851-855)Meta(939-944)Meta(1173-1177)Meta(1330-1333)Meta(1469-1477)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (1)
RadiusUserGroup(58-60)
openwisp_radius/tests/test_api/test_api.py (5)
openwisp_radius/api/serializers.py (1)
user(728-729)openwisp_radius/models.py (2)
RadiusGroup(43-46)RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (2)
RadiusGroup(43-45)RadiusUserGroup(58-60)openwisp_radius/tests/__init__.py (1)
_create_radius_usergroup(123-128)openwisp_radius/base/models.py (2)
delete(686-689)delete(1076-1079)
openwisp_radius/api/views.py (2)
openwisp_radius/api/serializers.py (11)
RadiusUserGroupSerializer(368-395)user(728-729)Meta(141-144)Meta(294-297)Meta(304-306)Meta(347-350)Meta(369-372)Meta(399-402)Meta(408-410)Meta(505-508)Meta(766-781)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)
🔇 Additional comments (2)
openwisp_radius/api/serializers.py (1)
374-395: Good user-scoped serializer enforcement.Constraining
groupqueryset by parent-user organizations and injecting parent user context in validation is a solid guardrail for nested endpoint integrity.openwisp_radius/api/views.py (1)
952-1020: Nice separation of concerns for nested RadiusUserGroup endpoints.
BaseRadiusUserGroupViewcleanly centralizes parent-user and organization scoping, and the list/create view wiring is consistent and maintainable.
pandafy
force-pushed
the
issues/676-api-radius-user-group
branch
from
1a139d2 to
c8fa5f9
Compare
|
@coderabbitai review and close all addressed inline comments. |
|
✅ Actions performedComments resolved and changes approved. Review triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/user/rest-api.rst (1)
983-983:⚠️ Potential issue | 🟡 MinorUse “filtering” (not “sorting”) in this endpoint description.
Line 983 describes query-parameter filtering behavior, not ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/rest-api.rst` at line 983, The sentence "It supports sorting by organization id and organization slug." is incorrect — replace "sorting" with "filtering" so it reads "It supports filtering by organization id and organization slug." to correctly describe the query-parameter behavior in the REST API docs; update that exact sentence in the REST API endpoint description to avoid implying ordering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/api/views.py`:
- Around line 963-967: get_queryset currently only filters by user_id and can
return RadiusUserGroup rows from organizations the requester doesn't manage;
update get_queryset to, after obtaining qs = super().get_queryset(),
additionally restrict qs to organizations managed by the requesting user unless
request.user.is_superuser is True. Use request.user.is_superuser to skip scoping
for superusers and filter by the requesting user's managed organizations (e.g.,
request.user.managed_organizations or an equivalent method/queryset) when
calling qs.filter(user_id=self.kwargs["user_pk"], organization__in=...) so only
assignments within managed orgs are returned.
---
Duplicate comments:
In `@docs/user/rest-api.rst`:
- Line 983: The sentence "It supports sorting by organization id and
organization slug." is incorrect — replace "sorting" with "filtering" so it
reads "It supports filtering by organization id and organization slug." to
correctly describe the query-parameter behavior in the REST API docs; update
that exact sentence in the REST API endpoint description to avoid implying
ordering behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/user/rest-api.rstopenwisp_radius/api/serializers.pyopenwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_radius/tests/test_api/test_api.py (5)
openwisp_radius/api/serializers.py (2)
RadiusUserGroupSerializer(368-397)user(730-731)openwisp_radius/models.py (2)
RadiusGroup(43-46)RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (2)
RadiusGroup(43-45)RadiusUserGroup(58-60)openwisp_radius/tests/__init__.py (1)
_create_radius_usergroup(123-128)tests/openwisp2/sample_users/models.py (1)
OrganizationUser(37-39)
openwisp_radius/api/serializers.py (3)
openwisp_radius/api/views.py (6)
Meta(424-427)Meta(870-871)Meta(993-995)get(166-178)get(707-719)get_parent_queryset(969-971)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (1)
RadiusUserGroup(58-60)
openwisp_radius/api/views.py (2)
openwisp_radius/api/serializers.py (11)
RadiusUserGroupSerializer(368-397)user(730-731)Meta(141-144)Meta(294-297)Meta(304-306)Meta(347-350)Meta(369-372)Meta(401-404)Meta(410-412)Meta(507-510)Meta(768-783)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)
🔇 Additional comments (2)
openwisp_radius/api/serializers.py (1)
374-397: Good defensive initialization and tenant-scoped validation path.Line 374 onward now safely handles missing nested view context and keeps write choices constrained.
openwisp_radius/tests/test_api/test_api.py (1)
1296-1527: Solid coverage for new RadiusUserGroup API surface.The added list/detail/serializer tests exercise permissions, tenancy paths, CRUD flows, and no-view serializer initialization well.
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/rest-api.rst`:
- Around line 1023-1033: The PUT parameters table is misleading because it lists
both fields as optional; update the docs for the PUT operation to state that
"group" is required for full updates (no default on the model) while "priority"
remains optional, or alternatively document that PUT supports partial updates if
the endpoint actually accepts missing fields; specifically adjust the PUT
section and the Params table entries for "group" and "priority" to reflect the
real behavior (mention "group" is required for full update on the RADIUS user
group assignment model and keep "priority" as optional) so readers of the PUT
documentation see accurate validation requirements.
In `@openwisp_radius/api/views.py`:
- Around line 996-1006: The class RadiusUserGroupFilter currently sets
organization_slug = None which overrides the parent OrganizationManagedFilter
attribute; add a brief inline comment next to the organization_slug assignment
in RadiusUserGroupFilter explaining that this intentionally disables the
inherited slug-based filter (because this filter uses group__organization and
group__organization__slug instead) so future maintainers understand the
override, or remove the assignment if it was unintentional.
In `@openwisp_radius/tests/test_api/test_api.py`:
- Around line 1424-1527: Add a subTest inside test_radius_user_group_detail that
issues a PUT to the existing url variable with a payload omitting the "group"
field (e.g., {"priority": 4}) and assert the API's expected behavior (most
likely a 400 Bad Request if PUT requires a full representation); refresh rug
from DB and verify that the group was not changed (or adjust the expected status
if your API treats PUT like PATCH and returns 200 and keeps group unchanged).
Use the existing radius_user_group_detail endpoint, the rug object
(RadiusUserGroup) and org1_default_group/org1_power_users_group fixtures to
locate the resource and validate the outcome.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/user/rest-api.rstopenwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_radius/tests/test_api/test_api.py (1)
openwisp_radius/api/serializers.py (2)
RadiusUserGroupSerializer(368-397)user(730-731)
openwisp_radius/api/views.py (4)
openwisp_radius/api/serializers.py (12)
RadiusUserGroupSerializer(368-397)get_queryset(416-421)user(730-731)Meta(141-144)Meta(294-297)Meta(304-306)Meta(347-350)Meta(369-372)Meta(401-404)Meta(410-412)Meta(507-510)Meta(768-783)openwisp_radius/models.py (1)
RadiusUserGroup(61-64)tests/openwisp2/sample_radius/models.py (1)
RadiusUserGroup(58-60)openwisp_radius/api/freeradius_views.py (1)
get_queryset(492-493)
🔇 Additional comments (5)
docs/user/rest-api.rst (1)
964-1050: Documentation for RADIUS User Groups endpoints looks well-structured.The new documentation section follows the established patterns in this file, covers all CRUD operations, and properly documents the organization filtering capabilities and constraints.
openwisp_radius/api/views.py (2)
952-994: Well-implemented base view with proper multi-tenancy controls.The
BaseRadiusUserGroupViewcorrectly implements:
- Organization-scoped access for non-superusers via
get_queryset(lines 973-977)- Parent user filtering via
get_parent_queryset- Dynamic filter construction that works with swappable user models
The cross-tenant access concern from previous reviews has been properly addressed.
1024-1069: LGTM!Both
RadiusUserGroupListCreateViewandRadiusUserGroupDetailVieware properly implemented with:
- Appropriate pagination and filtering for the list view
- Correct
organization_fieldfor permission checks on the detail view- Comprehensive Swagger documentation for all HTTP methods
openwisp_radius/tests/test_api/test_api.py (2)
1296-1422: Comprehensive test coverage for RadiusUserGroup list/create operations.The test thoroughly covers:
- Authentication requirements
- Permission checks
- Basic list and filtering functionality
- Create operations with organization constraints
- Cross-organization access restrictions
- Superuser override capabilities
The fix from the past review (ensuring
admin_usergets org2 membership at line 1406, nottarget_user) has been correctly applied.
1528-1531: Good edge case test for serializer initialization.This test validates the defensive behavior when the serializer is instantiated without view context, ensuring the group queryset is properly restricted to empty.
openwisp_radius/api/views.py
Outdated
| ) | ||
|
|
||
| def get_parent_queryset(self): | ||
| """Get the parent user from the URL.""" |
There was a problem hiding this comment.
| """Get the parent user from the URL.""" |
|
@coderabbitai review, focus on real issues |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_radius/api/views.py`:
- Around line 957-971: RadiusUserGroupDetailView's object-level check uses
organization_field on the pre-update object but
RadiusUserGroupSerializer.__init__ currently scopes the group field to the
target user's organizations, allowing a manager to reassign a relation to an org
they don't manage; fix by further filtering the serializer's group queryset to
the request user's managed organizations (use
self.context["view"].request.user.organizations_managed) so the group choices
are limited to groups whose organization is in that set, and add a regression
test that creates a target user in two orgs, logs in as an org1 manager,
attempts to PUT/PATCH the user-group relation to a group in org2 and asserts the
update is rejected (403 or validation error).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb98f6c7-9d67-452c-9434-5f4a32b6419c
📒 Files selected for processing (2)
openwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
📚 Learning: 2026-03-02T16:47:07.405Z
Learnt from: pandafy
Repo: openwisp/openwisp-radius PR: 680
File: docs/user/rest-api.rst:1023-1033
Timestamp: 2026-03-02T16:47:07.405Z
Learning: In the RADIUS User Groups API endpoint (`/api/v1/users/user/<user_pk>/radius-group/<uuid>/`), the `group` parameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
Applied to files:
openwisp_radius/api/views.pyopenwisp_radius/tests/test_api/test_api.py
🔇 Additional comments (1)
openwisp_radius/tests/test_api/test_api.py (1)
1487-1496: Good regression coverage for PUT-as-partial-update behavior.This locks in the API contract that
groupstays optional on PUT, which is easy to regress when the serializer or update flow changes.Based on learnings, in the RADIUS User Groups API endpoint (
/api/v1/users/user/<user_pk>/radius-group/<uuid>/), thegroupparameter is optional in PUT requests, allowing partial updates rather than requiring all fields for a full update.
openwisp_radius/api/serializers.py
Outdated
| self._user = None | ||
| if self._user: | ||
| self.fields["group"].queryset = self.fields["group"].queryset.filter( | ||
| organization_id__in=self._user.organizations_dict.keys() |
There was a problem hiding this comment.
@pandafy after reading the last comment from coderabbit I realized this may be indeed wrong, shouldn't we use self._user.organizations_managed here?
There was a problem hiding this comment.
@nemesifier you are correct, I have fixed this in 944c990
CI Failure AnalysisHello @pandafy, There are multiple test failures and performance issues identified in the CI logs. Failures & Remediation
|
pandafy
force-pushed
the
issues/676-api-radius-user-group
branch
from
944c990 to
81714c6
Compare
CI Failure Analysis (commit 81714c6)Hello @pandafy, (Analysis for commit 81714c6) The CI run failed due to a test execution error, specifically related to slow tests. Failure: Test Execution Timeout Explanation: Remediation:
|
openwisp_radius/api/serializers.py
Outdated
| self.fields["group"].queryset = ( | ||
| self.fields["group"] | ||
| .queryset.filter(organization__in=orgs) | ||
| .filter(organization__in=self._user.organizations_dict.keys()) |
There was a problem hiding this comment.
Here the code filters using user.organizations_dict.keys() but above user.organizations_managed is used.
Is this difference intentional and if so what's the motivation?
Keep in mind organizations_dict.keys() returns all the organizations the user is associated with, irregardless of whether the user is manager, owner or end user.
The discrepancy doesn't look intentional, if it is we need to add a comment explaining the nuances.
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
3 participants
Checklist
Reference to Existing Issue
Closes #676