[feature] Allowed read only access of shared objects to non-superusers #238#444
[feature] Allowed read only access of shared objects to non-superusers #238#444
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
pandafy
force-pushed
the
issues/238-view-shared-objects
branch
3 times, most recently
from
0929c13 to
80511df
Compare
pandafy
force-pushed
the
issues/238-view-shared-objects
branch
2 times, most recently
from
e72b20e to
c1da85c
Compare
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I haven't performed manual testing yet, I should be able to do this tomorrow.
In the meantime I have looked at the code and left some comments below.
I also noticed two more important points:
- Docs: the documentation has a section dedicated to shared objects, I think it's worth to update the text to reflect the functionality achieved in this PR.
- DRF 1.6: did you address this point which is listed in the issue description?
tests/testapp/tests/test_selenium.py
Outdated
| def test_organization_autocomplete_filter(self): | ||
| """ | ||
| The autocomplete_filter should show option to filter | ||
| shared objects to non-superuser. |
There was a problem hiding this comment.
I am not sure I fully understand this docstring/comment: should the option to filter shared objects appear only to users who have the is_superuser flag set to False? Or what does this text mean exactly? Let's make it unambiguous.
| django-phonenumber-field~=8.1.0 | ||
| phonenumbers~=9.0.4 | ||
| openwisp-utils[rest,celery] @ https://github.com/openwisp/openwisp-utils/tarball/1.2 | ||
| openwisp-utils[rest,celery] @ https://github.com/openwisp/openwisp-utils/tarball/bump-drf |
There was a problem hiding this comment.
TODO: Revert this before merging
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
This is what I found during testing in openwisp-controller:
openwisp/openwisp-controller#1024 (review)
We'll also need a way to protect some sensitive fields like credentials parameters, can you create a subissue for this and link it to the parent issue please? We should work on that as soon as this PR is ready.
pandafy
changed the title
pandafy
moved this from To do (general)
to Needs review
in OpenWISP Contributor's Board
pandafy
moved this from In progress
to Ready for review/testing
in OpenWISP Priorities for next releases
docs/user/basic-concepts.rst
Outdated
| objects. However, they can use these shared objects when creating related | ||
| organization-specific resources. E.g., an organization manager can use a | ||
| shared VPN server to create a configuration template for their | ||
| organization. |
There was a problem hiding this comment.
Let's mention this works both in admin and REST API, let's mention that non superusers may be prevented to viewing sensitive details of certain shared objects which would allow them to gain unauthorized access to resources used by other organizations.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks good! Let's squash the commits into 1 so we can then add new commits on this branch for the follow up issues. Let's not merge this yet to avoid introducing potential security issues in the development version.
pandafy
force-pushed
the
issues/238-view-shared-objects
branch
from
fd88b21 to
4c181e0
Compare
pandafy
force-pushed
the
issues/238-view-shared-objects
branch
from
4c181e0 to
65c6b17
Compare
pandafy
force-pushed
the
issues/238-view-shared-objects
branch
from
b1a8dc9 to
e81b36f
Compare
pandafy
force-pushed
the
issues/238-view-shared-objects
branch
from
e81b36f to
f348ee4
Compare
openwisp_users/api/mixins.py
Outdated
| and not request.user.is_superuser | ||
| and "organization" in obj | ||
| and obj["organization"] is None |
There was a problem hiding this comment.
WIP: The organization field will be made configurable.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request implements a shared objects access control system allowing non-superusers to view shared objects (objects without an organization assignment) in read-only mode while preventing unauthorized creation. The changes introduce a new Sequence Diagram(s)sequenceDiagram
actor NonSuperUser as Non-Superuser Org Admin
actor SuperUser as Superuser
participant View as View/Serializer
participant Mixin as HideSensitiveFieldsMixin
participant Query as QuerySet Filter
participant Model as Model (shared_obj)
participant Response as Response
rect rgba(100, 150, 200, 0.5)
Note over NonSuperUser,Response: Non-Superuser Access to Shared Object
NonSuperUser->>View: GET /api/templates/1/
View->>Query: get_organization_queryset()
Query->>Query: Check: is_superuser?
Query-->>Query: No → filter by org + include org=None
Query->>Model: Retrieve shared object
Model-->>View: Return shared object data
View->>Mixin: to_representation()
Mixin->>Mixin: _is_object_shared()?
Mixin-->>Mixin: Yes → org=None detected
Mixin->>Mixin: hide_sensitive_fields()
Mixin-->>Mixin: Remove 'secrets' field
View-->>NonSuperUser: JSON (name, organization fields only)
end
rect rgba(150, 200, 100, 0.5)
Note over SuperUser,Response: Superuser Access to Shared Object
SuperUser->>View: GET /api/templates/1/
View->>Query: get_organization_queryset()
Query->>Query: Check: is_superuser?
Query-->>Query: Yes → no filtering
Query->>Model: Retrieve shared object
Model-->>View: Return shared object data
View->>Mixin: to_representation()
Mixin->>Mixin: _is_object_shared()?
Mixin-->>Mixin: is_superuser=True
Mixin-->>Mixin: Skip field hiding
View-->>SuperUser: JSON (all fields including 'secrets')
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 Fix all issues with AI agents
In `@openwisp_users/api/mixins.py`:
- Around line 157-200: Remove the unused helper _is_object_shared method from
the HideSensitiveFieldsMixin class—it's never called (hide_sensitive_fields
checks the serialized "organization" key directly) so delete the entire
_is_object_shared definition and its docstring; keep hide_sensitive_fields,
get_sensitive_fields, and to_representation as-is, and run tests/lint to ensure
no external references to _is_object_shared remain (check views or tests that
might have referenced _is_object_shared by name before removing).
In `@openwisp_users/tests/test_api/__init__.py`:
- Around line 182-201: The helper assert_sensitive_fields_visibility should
explicitly check that the target object was found in the list response before
using item; in the function (assert_sensitive_fields_visibility) after iterating
results to locate the entry with str(item["id"]) == str(obj.pk), add a boolean
found flag (or assert) tied to listview_name and obj.pk and fail with a clear
message if not found, then proceed to assert each field in sensitive_fields on
the located item; this prevents using an undefined item and surfaces a
meaningful failure when the object is absent from the list.
In `@requirements.txt`:
- Line 6: Replace the mutable branch tarball dependency for openwisp-utils with
the stable pinned release: change the line referencing
"openwisp-utils[rest,celery] @
https://github.com/openwisp/openwisp-utils/tarball/bump-drf" in requirements.txt
to a fixed release specifier such as "openwisp-utils[rest,celery]==1.2.0"; if
your CI/build system requires an integrity hash, use the release tarball URL for
v1.2.0 and include the appropriate hash instead of the branch tarball URL to
ensure reproducible installs.
🧹 Nitpick comments (2)
openwisp_users/multitenancy.py (1)
120-132: LGTM - Permission enforcement for shared objects.The implementation correctly prevents non-superusers from changing or deleting shared objects by:
- Delegating to the parent permission check first
- Verifying the object belongs to an organization managed by the user
- Returning False for shared objects (where
organization_idis None)Note: The docstring for
has_delete_permissionmentions "cannot change" but should say "cannot delete" for consistency.📝 Minor docstring fix
def has_delete_permission(self, request, obj=None): """ Returns True if the user has permission to delete the object. - Non-superusers cannot change shared objects. + Non-superusers cannot delete shared objects. """ return self._has_org_permission(request, obj, super().has_delete_permission)openwisp_users/tests/utils.py (1)
293-325: Consider using more specific assertions for sensitive fields.At lines 312 and 318,
assertNotContains(response, field)andassertContains(response, field)check for field names as plain strings. This could lead to false positives if the field name appears elsewhere in the response (e.g., "host" matching "ghost" or appearing in unrelated text).Consider using more specific patterns or checking the form fields directly for more robust assertions.
💡 More specific field assertion example
# Instead of checking for bare field name, check for form field pattern self.assertNotContains(response, f'name="{field}"') # Or check the form's fields attribute if available
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/user/basic-concepts.rstopenwisp_users/api/mixins.pyopenwisp_users/multitenancy.pyopenwisp_users/static/admin/js/autocomplete.jsopenwisp_users/tests/test_api/__init__.pyopenwisp_users/tests/utils.pyopenwisp_users/views.pyrequirements.txttests/testapp/migrations/0007_template_secrets.pytests/testapp/models.pytests/testapp/tests/mixins.pytests/testapp/tests/test_admin.pytests/testapp/tests/test_filter_classes.pytests/testapp/tests/test_permission_classes.pytests/testapp/tests/test_selenium.py
🧰 Additional context used
🧬 Code graph analysis (7)
tests/testapp/tests/test_filter_classes.py (2)
openwisp_users/tests/test_api/__init__.py (1)
APITestCase(233-234)tests/testapp/__init__.py (1)
CreateMixin(1-40)
openwisp_users/tests/test_api/__init__.py (3)
openwisp_users/tests/utils.py (5)
TestMultitenantAdminMixin(176-362)_create_administrator(103-122)_get_org(124-128)_get_admin(136-140)_test_sensitive_fields_visibility_on_shared_and_org_objects(293-325)openwisp_users/api/views.py (2)
post(66-67)put(148-149)tests/testapp/views.py (2)
get(60-66)get(95-100)
openwisp_users/multitenancy.py (3)
openwisp_users/api/mixins.py (2)
get_sensitive_fields(163-168)org_field(18-19)openwisp_users/base/models.py (1)
organizations_managed(164-165)openwisp_users/admin.py (1)
queryset(467-472)
openwisp_users/api/mixins.py (3)
openwisp_users/api/views.py (1)
get_organization_queryset(217-226)openwisp_users/multitenancy.py (1)
get_sensitive_fields(24-25)openwisp_users/api/serializers.py (2)
to_representation(138-139)to_representation(207-217)
openwisp_users/tests/utils.py (1)
openwisp_users/tests/test_api/__init__.py (1)
_test_sensitive_fields_visibility_on_shared_and_org_objects(171-230)
tests/testapp/tests/test_admin.py (4)
openwisp_users/tests/utils.py (6)
TestMultitenantAdminMixin(176-362)TestOrganizationMixin(35-173)_test_org_admin_create_shareable_object(242-270)_test_object_organization_fk_autocomplete_view(327-352)_test_org_admin_view_shareable_object(272-291)_test_sensitive_fields_visibility_on_shared_and_org_objects(293-325)tests/testapp/__init__.py (1)
_create_template(18-24)openwisp_users/api/serializers.py (4)
update(98-134)update(166-172)update(327-359)save(453-457)tests/testapp/models.py (1)
Template(9-21)
tests/testapp/tests/test_selenium.py (4)
openwisp_users/tests/utils.py (2)
_create_org(64-68)_create_administrator(103-122)openwisp_users/base/models.py (1)
name(273-279)openwisp_users/models.py (1)
Organization(24-26)tests/openwisp2/sample_users/models.py (1)
Organization(49-50)
🪛 Ruff (0.14.13)
tests/testapp/migrations/0007_template_secrets.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_users/multitenancy.py
24-24: Unused method argument: request
(ARG002)
24-24: Unused method argument: obj
(ARG002)
tests/testapp/models.py
10-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/testapp/tests/test_selenium.py
159-159: Possible hardcoded password assigned to argument: "password"
(S106)
169-169: Possible hardcoded password assigned to argument: "password"
(S106)
🔇 Additional comments (30)
openwisp_users/views.py (1)
44-47: LGTM!The logic correctly allows non-superusers to see the "Shared systemwide" option in autocomplete dropdowns only when filtering (via
is_filterparameter), while preventing them from selecting it during object creation. This aligns well with the PR objective of allowing read-only access to shared objects.docs/user/basic-concepts.rst (1)
246-257: LGTM!The documentation clearly explains that:
- Shared objects can only be created by superusers
- Non-superusers have view-only access in both admin interface and REST API
- Non-superusers can use shared objects when creating organization-specific resources
- Sensitive details may be restricted to prevent unauthorized access
This addresses the feedback from the previous review requesting these clarifications.
openwisp_users/static/admin/js/autocomplete.js (1)
1-39: LGTM!Clean implementation that:
- Properly extends Django's autocomplete behavior with the
is_filterparameter- Uses jQuery plugin pattern correctly
- Excludes template elements (
__prefix__) to avoid initializing placeholders- Handles dynamically added formsets via the
formset:addedeventopenwisp_users/multitenancy.py (4)
24-25: Unused parameters are acceptable for API consistency.The static analysis flags
requestandobjas unused, but these parameters maintain a consistent method signature that subclasses may need to override with custom logic based on request context or the specific object.
43-56: LGTM - Sensitive field filtering logic.The implementation correctly:
- Only filters fields when viewing an existing object (
objis truthy)- Traverses to the parent object when
multitenant_parentis set- Hides sensitive fields only for shared objects (where
organization_idis None)- Only applies restrictions to non-superusers
103-118: The type consistency is already correctly handled in the implementation.At line 147 in
openwisp_users/base/models.py, organization IDs are explicitly converted to strings when building theorganizations_dict:org_id = str(org.pk). These string keys are then appended to the list returned byorganizations_managed, which means the property returns a list of string organization IDs. The comparison at line 116 (str(obj.organization_id) in request.user.organizations_managed) is therefore type-consistent—comparing a string against a list of strings—and requires no changes.Likely an incorrect or invalid review comment.
87-101: The logic is correct, but the characterization in the review comment is misleading.The condition
and not self.multitenant_shared_relationsmeans filtering is applied only when the list is empty. This is intentional: when a relation is marked as "shared" (present inmultitenant_shared_relations), it should be visible across organization boundaries, so organization-based filtering is skipped. Conversely, when no relations are marked as shared (empty list), organization filtering is applied to restrict results to the user's managed organizations.The edge case when
field_nameis absent from the request is already handled correctly—the first conditionrequest.GET.get("field_name")evaluates to falsy, so the entire filter is bypassed, and the queryset already filtered byget_queryset()is used instead.openwisp_users/api/mixins.py (3)
28-36: LGTM - Dynamic lookup field for shared objects.Good refactoring to use the dynamic
organization_lookuppath instead of hardcodingorganization__isnull. This properly handles models where the organization relationship is through a parent object.
141-154: LGTM - FilterByParent classes now include shared objects.Adding
SharedObjectsLookuptoFilterByParentManagedandFilterByParentOwnedensures that non-superusers can access shared objects through parent relationships, aligning with the PR objective.
247-274: LGTM - Serializer classes now hide sensitive fields.All three
FilterSerializerByOrg*classes correctly inheritHideSensitiveFieldsMixinfirst in the MRO, ensuringto_representationproperly hides sensitive fields for shared objects.openwisp_users/tests/utils.py (3)
242-270: LGTM - Comprehensive test helper for shareable object creation.Good test utility that:
- Verifies non-superusers cannot create shared objects
- Handles Django 5.2+ error message format changes
- Provides flexibility with
raises_erroranderror_messageparameters- Validates expected object count after attempted creation
327-352: LGTM - Autocomplete view test helper.Well-structured test that verifies:
- Org admins only see organizations they manage
- Org admins don't see the "Shared systemwide" option
- Superusers see all organizations including the shared option
354-361: LGTM - Updated autocomplete path helper.The
is_filterparameter integration properly appends the query parameter when needed, aligning with the frontend autocomplete.js changes.openwisp_users/tests/test_api/__init__.py (3)
15-89: Solid shared-object CRUD helper.
Clear separation between list and detail flows, with consistent auth handling.
90-169: Good defaults for org-user vs superuser access.
The status-code presets make test intent easy to read.
233-233: LGTM: APITestCase consolidation.
Nice to expose the API helpers through a single base.tests/testapp/tests/mixins.py (1)
1-6: LGTM: mixin now uses the API test helper.
Keeps test utilities aligned with the new shared-object API mixin.tests/testapp/tests/test_selenium.py (2)
26-38: Nice refactor: configurable Select2 selector.
This makes the helper usable across different autocomplete widgets.
151-175: Good coverage for shared-option visibility in filters.
The test aligns with the shared-object visibility requirement.tests/testapp/tests/test_permission_classes.py (2)
7-17: LGTM: base test class aligns with new API mixin.
The new inheritance matches the updated test utilities.
234-311: Great: shared-object access + sensitive-field visibility covered.
These cases exercise the core behavior introduced in this PR.tests/testapp/tests/test_filter_classes.py (1)
7-19: LGTM: test base updated to API-aware mixin.
Keeps filter tests consistent with the rest of the suite.tests/testapp/models.py (1)
9-15: LGTM: secrets field + sensitive_fields metadata.
This provides a clear target for the sensitive-field visibility tests.tests/testapp/migrations/0007_template_secrets.py (1)
1-18: LGTM!Standard Django migration adding the
secretsTextField to support the new sensitive field visibility feature. The field definition correctly matches the model intests/testapp/models.py.Note: The static analysis hints about
ClassVarannotations are false positives—dependenciesandoperationsare idiomatic Django migration attributes that should remain as-is.tests/testapp/tests/test_admin.py (6)
8-8: LGTM!Import correctly includes both mixins needed by the test classes in this file.
47-58: LGTM!The class now correctly inherits from
TestMultitenantAdminMixinto leverage shared object testing utilities. The_create_templatehelper is well-structured with sensible defaults and proper validation viafull_clean()beforesave().
60-68: LGTM!Good refactoring to use the reusable
_test_org_admin_create_shareable_objecthelper. This aligns with the PR objective of creating reusable test utilities and validates that non-superusers cannot create shared objects.
70-73: LGTM!New test validates organization FK autocomplete behavior for the Template model using the shared test utility.
75-93: LGTM!Good test coverage for shared object access:
test_org_admin_view_shared_templatevalidates read-only access for non-superusers.test_superuser_create_shareable_templateconfirms superusers retain the ability to create shared objects.The change from
_create_admin()to_get_admin()is appropriate when usingTestMultitenantAdminMixin, which manages admin user setup.
95-112: LGTM!Comprehensive test for sensitive field visibility control. Correctly tests both shared and organization-scoped objects using the Template model's
sensitive_fields = ["secrets"]attribute. The test aligns with the PR objective of hiding sensitive fields for non-superusers viewing shared objects.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| class HideSensitiveFieldsMixin: | ||
| """ | ||
| Mixin to hide sensitive fields in the serializer representation | ||
| based on the organization of the user. | ||
| """ | ||
|
|
||
| def get_sensitive_fields(self): | ||
| """ | ||
| Returns a list of sensitive fields that should be hidden. | ||
| """ | ||
| ModelClass = self.Meta.model | ||
| return getattr(ModelClass, "sensitive_fields", []) | ||
|
|
||
| def _is_object_shared(self, instance): | ||
| """ | ||
| Returns the organization of the instance if it exists. | ||
| """ | ||
| view = self.context.get("view") | ||
| organization_field = getattr(view, "organization_field", "organization_id") | ||
| related_field = instance | ||
| for field in organization_field.split("__"): | ||
| if hasattr(related_field, field): | ||
| related_field = getattr(related_field, field) | ||
| else: | ||
| return False | ||
| return related_field is None | ||
|
|
||
| def hide_sensitive_fields(self, obj): | ||
| request = self.context.get("request") | ||
| if ( | ||
| request | ||
| and not request.user.is_superuser | ||
| and "organization" in obj | ||
| and obj["organization"] is None | ||
| ): | ||
| for field in self.get_sensitive_fields(): | ||
| if field in obj: | ||
| del obj[field] | ||
| return obj | ||
|
|
||
| def to_representation(self, data): | ||
| rep = super().to_representation(data) | ||
| self.hide_sensitive_fields(rep) | ||
| return rep |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the repository structure
git ls-files | grep -E "(serializer|mixin)" | head -20Repository: openwisp/openwisp-users
Length of output: 190
🏁 Script executed:
# Find the actual file mentioned in the review
find . -type f -name "mixins.py" | grep -E "api|openwisp_users"Repository: openwisp/openwisp-users
Length of output: 124
🏁 Script executed:
# Check for serializer classes and their organization field handling
rg "class.*Serializer.*:" --type py -A 20 | head -100Repository: openwisp/openwisp-users
Length of output: 5836
🏁 Script executed:
# Find where HideSensitiveFieldsMixin is used
rg "HideSensitiveFieldsMixin" --type py -B 2 -A 5Repository: openwisp/openwisp-users
Length of output: 1689
🏁 Script executed:
# Find models with sensitive_fields attribute
rg "sensitive_fields" --type py -B 2 -A 2Repository: openwisp/openwisp-users
Length of output: 9122
🏁 Script executed:
# Look at the actual mixins.py file to understand context better
cat -n ./openwisp_users/api/mixins.py | head -250Repository: openwisp/openwisp-users
Length of output: 9385
🏁 Script executed:
# Check if there are any views that override organization_field
rg "organization_field" --type py -B 2 -A 2Repository: openwisp/openwisp-users
Length of output: 5937
🏁 Script executed:
# Look at actual serializers to see what fields they include
cat -n ./tests/testapp/serializers.py | head -100Repository: openwisp/openwisp-users
Length of output: 3254
🏁 Script executed:
# Check the Template model and its serializer
rg "class Template" --type py -A 10 | head -50Repository: openwisp/openwisp-users
Length of output: 2491
🏁 Script executed:
# Look for test cases that verify the sensitive fields behavior
cat -n ./openwisp_users/tests/test_api/__init__.py | sed -n '1,100p'Repository: openwisp/openwisp-users
Length of output: 4727
🏁 Script executed:
# Find ShareableOrgMixin definition
rg "class ShareableOrgMixin" --type py -A 15Repository: openwisp/openwisp-users
Length of output: 736
🏁 Script executed:
# Check what fields Template includes
rg "class Template" --type py -B 5 -A 20 | grep -A 20 "class Template"Repository: openwisp/openwisp-users
Length of output: 4480
🏁 Script executed:
# Look at the full test for sensitive fields visibility
rg "_test_sensitive_fields_visibility_on_shared_and_org_objects" --type py -A 50 | head -80Repository: openwisp/openwisp-users
Length of output: 5883
🏁 Script executed:
# Check if there's a specific test for nested organization_field with sensitive fields
rg "test_template_sensitive_fields" --type py -A 30Repository: openwisp/openwisp-users
Length of output: 1590
🏁 Script executed:
# Check if _is_object_shared method is used anywhere
rg "_is_object_shared" --type pyRepository: openwisp/openwisp-users
Length of output: 138
Remove unused _is_object_shared method.
The implementation correctly hides sensitive fields for shared (organization=None) objects by directly checking the "organization" key in the serialized representation. However, the _is_object_shared method (lines 170–182) is defined but never called; hide_sensitive_fields doesn't use it. The method can be safely removed to eliminate dead code.
Regarding the nested organization field concern: while organization_field on views can reference nested paths like template__organization, this is used only for permission filtering on the model instance. The serialized output always includes "organization" at the top level from the ShareableOrgMixin, so the direct check in hide_sensitive_fields works correctly across all models.
🤖 Prompt for AI Agents
In `@openwisp_users/api/mixins.py` around lines 157 - 200, Remove the unused
helper _is_object_shared method from the HideSensitiveFieldsMixin class—it's
never called (hide_sensitive_fields checks the serialized "organization" key
directly) so delete the entire _is_object_shared definition and its docstring;
keep hide_sensitive_fields, get_sensitive_fields, and to_representation as-is,
and run tests/lint to ensure no external references to _is_object_shared remain
(check views or tests that might have referenced _is_object_shared by name
before removing).
| def assert_sensitive_fields_visibility(obj, user, should_be_visible=False): | ||
| token = self._obtain_auth_token(user.username, "tester") | ||
| auth = {"HTTP_AUTHORIZATION": f"Bearer {token}"} | ||
| # List view | ||
| listview_path = reverse(listview_name) | ||
| response = self.client.get(listview_path, **auth) | ||
| self.assertEqual(response.status_code, 200) | ||
| results = ( | ||
| response.data | ||
| if "results" not in response.data | ||
| else response.data["results"] | ||
| ) | ||
| for item in results: | ||
| if str(item["id"]) == str(obj.pk): | ||
| break | ||
| for field in sensitive_fields: | ||
| self.assertEqual( | ||
| field in item, | ||
| should_be_visible, | ||
| ) |
There was a problem hiding this comment.
Guard against missing list entry before asserting sensitive fields.
If the target object is absent from the list response, Line 194 onward can use an undefined item, masking the real failure. Add an explicit “found” check.
🛠️ Proposed fix
- for item in results:
- if str(item["id"]) == str(obj.pk):
- break
- for field in sensitive_fields:
- self.assertEqual(
- field in item,
- should_be_visible,
- )
+ item = None
+ for row in results:
+ if str(row.get("id")) == str(obj.pk):
+ item = row
+ break
+ self.assertIsNotNone(item, "Expected object not found in list response")
+ for field in sensitive_fields:
+ self.assertEqual(field in item, should_be_visible)🤖 Prompt for AI Agents
In `@openwisp_users/tests/test_api/__init__.py` around lines 182 - 201, The helper
assert_sensitive_fields_visibility should explicitly check that the target
object was found in the list response before using item; in the function
(assert_sensitive_fields_visibility) after iterating results to locate the entry
with str(item["id"]) == str(obj.pk), add a boolean found flag (or assert) tied
to listview_name and obj.pk and fail with a clear message if not found, then
proceed to assert each field in sensitive_fields on the located item; this
prevents using an undefined item and surfaces a meaningful failure when the
object is absent from the list.
| django-phonenumber-field~=8.1.0 | ||
| phonenumbers~=9.0.8 | ||
| openwisp-utils[rest,celery] @ https://github.com/openwisp/openwisp-utils/tarball/1.2 | ||
| openwisp-utils[rest,celery] @ https://github.com/openwisp/openwisp-utils/tarball/bump-drf |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
openwisp-utils releases tags stable version
💡 Result:
The current stable openwisp-utils release tag is 1.2.0 (refs/tags/1.2.0). [1][2]
Sources:
- PyPI release metadata showing tag refs/tags/1.2.0. [1]
- piwheels / project release listing (shows 1.2.x series). [2]
🌐 Web query:
openwisp-utils bump-drf branch DRF changes
💡 Result:
The openwisp-utils codebase has been updated to bump Django REST Framework to ~=3.16.0 (this is reflected in the openwisp-utils 1.2 / OpenWISP 25.10 release notes). [1][2]
Sources:
- OpenWISP 25.10 — openwisp-utils (changes & dependencies) showing djangorestframework bumped to ~=3.16.0. [1]
- OpenWISP 25.10 release notes (module changelogs). [2]
🏁 Script executed:
cd . && git log --oneline -20 | head -20 && echo "---" && git branch -r 2>/dev/null | grep -i drf || echo "No remote branches matching 'drf'"Repository: openwisp/openwisp-users
Length of output: 132
🏁 Script executed:
cat requirements.txt | head -20Repository: openwisp/openwisp-users
Length of output: 333
Pin openwisp-utils to the stable release tag with a hash instead of a branch tarball.
The current dependency on openwisp-utils[rest,celery] @ https://github.com/openwisp/openwisp-utils/tarball/bump-drf uses a mutable branch-based URL, which breaks reproducibility and supply-chain guarantees. The DRF changes in bump-drf are already included in the stable release 1.2.0; switch to that tag instead:
openwisp-utils[rest,celery]==1.2.0
If a hash is required by your build system, include it in the URL.
🤖 Prompt for AI Agents
In `@requirements.txt` at line 6, Replace the mutable branch tarball dependency
for openwisp-utils with the stable pinned release: change the line referencing
"openwisp-utils[rest,celery] @
https://github.com/openwisp/openwisp-utils/tarball/bump-drf" in requirements.txt
to a fixed release specifier such as "openwisp-utils[rest,celery]==1.2.0"; if
your CI/build system requires an integrity hash, use the release tarball URL for
v1.2.0 and include the appropriate hash instead of the branch tarball URL to
ensure reproducible installs.
|
i was looking at the changes made here for a while and code rabbit and @ nemesifier have addressed everything important, (the dead ill close #478 👍🏼 |
4 participants
Checklist
Reference to Existing Issue
Closes #238
Closes #448
TODOS
-[x] DRF integration
-[x] Tests