[feature] Made disabled organizations readonly but deletable #522#542
[feature] Made disabled organizations readonly but deletable #522#542pandafy wants to merge 6 commits into
Conversation
Made disabled organizations read-only but deletable: objects belonging to a disabled organization can still be viewed and deleted, but not created or modified, in both the admin interface and the REST API. This also applies to the organization record itself (only re-enabling or unassigning its owner is allowed while disabled). Organization selection widgets exclude disabled organizations, while admin list filters keep them visible for auditing. Closes #522
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
|
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:
📝 WalkthroughWalkthroughAdds disabled-organization write protection across model validation, Django admin, and REST APIs. Disabled organizations remain readable and deletable, while updates, membership changes, ownership assignments, and new selections are restricted. Admin and API opt-outs are supported. Autocomplete and serializer querysets exclude inactive organizations, with transactional validation and rollback coverage. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminOrAPI
participant WriteGuard
participant Organization
Client->>AdminOrAPI: Request object creation or update
AdminOrAPI->>WriteGuard: Resolve related organization
WriteGuard->>Organization: Check is_active
Organization-->>WriteGuard: Active or disabled status
WriteGuard-->>AdminOrAPI: Permit, reject, or allow delete/read
AdminOrAPI-->>Client: Response or validation error
Possibly related issues
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous Review Summary (commit 4f84372)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4f84372)Status: No Issues Found | Recommendation: Merge Solid, well-scoped implementation of issue #522 (disabled organizations become read-only but still viewable and deletable). The change is consistent across the admin and REST API layers, and it comes with thorough regression coverage. A few things done well:
Files Reviewed (22 files)
Reviewed by step-3.7-flash · Input: 163.9K · Output: 34.4K · Cached: 1.6M |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
coverage: 98.012% (-0.1%) from 98.131% — issues/522-disabled-org into master |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_users/api/serializers.py (1)
316-340: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap
instance.full_clean()in_full_clean_or_raise()
openwisp_users/api/serializers.py:316-340—instance.full_clean()still raises DjangoValidationErrordirectly here, while the nestedOrganizationUserbranch already uses_full_clean_or_raise(). A model-level validation failure on the user will bubble up as a 500 instead of a DRF 400; use_full_clean_or_raise(instance)beforesave().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_users/api/serializers.py` around lines 316 - 340, In the create method, replace the direct instance.full_clean() call with _full_clean_or_raise(instance) before instance.save(), matching the OrganizationUser validation path and preserving the surrounding transaction flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_users/admin.py`:
- Around line 124-140: The add_fields method currently triggers a per-row
organization lookup through instance.organization. Update the inline formset’s
queryset to use select_related("organization") so each membership’s organization
is loaded with the initial query, while preserving the existing
inactive-organization field behavior in add_fields.
In `@openwisp_users/api/serializers.py`:
- Around line 220-232: The identical disabled-organization error message is
duplicated across serializer and mixin validation. Define one shared
translatable constant in an appropriate API module, update Organization
serializer’s default_error_messages to import and use it, and update the
organization mixin’s error_messages to use the same constant; apply these
changes in openwisp_users/api/serializers.py lines 220-232 and
openwisp_users/api/mixins.py lines 167-187.
In `@openwisp_users/base/models.py`:
- Around line 493-513: Update OrganizationUser.clean to fetch and compare the
persisted user_id alongside organization_id and is_admin in the no-op check.
Ensure changing the membership’s user on a disabled organization raises the
existing “Memberships of a disabled organization cannot be modified.”
ValidationError, while unchanged rows remain allowed.
In `@openwisp_users/multitenancy.py`:
- Around line 133-140: Update the organization queryset logic in the relevant
admin form setup to keep inactive organizations excluded by default, but include
the current object’s organization when disabled_organization_write_protection is
False. Use the existing request/form object context to identify that
organization, and update the documentation section in
docs/developer/admin-utils.rst covering this behavior to reflect the opt-out
exception; no other sites require changes.
---
Outside diff comments:
In `@openwisp_users/api/serializers.py`:
- Around line 316-340: In the create method, replace the direct
instance.full_clean() call with _full_clean_or_raise(instance) before
instance.save(), matching the OrganizationUser validation path and preserving
the surrounding transaction flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca155518-6f0d-4f87-8d43-31643b3a650c
📒 Files selected for processing (22)
docs/developer/admin-utils.rstdocs/developer/django-rest-framework-utils.rstdocs/user/basic-concepts.rstopenwisp_users/admin.pyopenwisp_users/api/mixins.pyopenwisp_users/api/permissions.pyopenwisp_users/api/serializers.pyopenwisp_users/apps.pyopenwisp_users/base/models.pyopenwisp_users/multitenancy.pyopenwisp_users/tests/test_admin.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_models.pyopenwisp_users/views.pyopenwisp_users/widgets.pytests/testapp/tests/test_admin.pytests/testapp/tests/test_multitenancy.pytests/testapp/tests/test_permission_classes.pytests/testapp/tests/test_selenium.pytests/testapp/tests/test_views.pytests/testapp/urls.pytests/testapp/views.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Place imports at the top of the file; only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside itFor Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_users/apps.pyopenwisp_users/widgets.pyopenwisp_users/views.pytests/testapp/tests/test_views.pytests/testapp/tests/test_admin.pytests/testapp/urls.pyopenwisp_users/api/permissions.pytests/testapp/tests/test_selenium.pyopenwisp_users/api/mixins.pyopenwisp_users/tests/test_models.pytests/testapp/tests/test_multitenancy.pytests/testapp/tests/test_permission_classes.pyopenwisp_users/base/models.pytests/testapp/views.pyopenwisp_users/multitenancy.pyopenwisp_users/api/serializers.pyopenwisp_users/admin.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_admin.py
🔇 Additional comments (22)
docs/developer/django-rest-framework-utils.rst (1)
134-196: LGTM!Also applies to: 314-322
openwisp_users/api/mixins.py (1)
4-16: LGTM!Also applies to: 325-325
openwisp_users/api/permissions.py (1)
2-2: LGTM!Also applies to: 98-122
openwisp_users/api/serializers.py (1)
24-36: LGTM!Also applies to: 75-83, 113-176, 396-422
openwisp_users/tests/test_api/test_api.py (1)
1-9: LGTM!Also applies to: 24-24, 120-155, 179-222, 661-695, 749-757, 788-820
tests/testapp/tests/test_permission_classes.py (1)
7-7: LGTM!Also applies to: 365-463
tests/testapp/views.py (1)
25-29: LGTM!Also applies to: 216-216, 227-248, 309-312
tests/testapp/urls.py (1)
62-71: LGTM!openwisp_users/base/models.py (1)
472-479: LGTM!Also applies to: 543-559
openwisp_users/apps.py (1)
198-201: LGTM!openwisp_users/tests/test_models.py (1)
413-523: LGTM!docs/user/basic-concepts.rst (1)
152-201: LGTM!tests/testapp/tests/test_admin.py (1)
50-51: LGTM!tests/testapp/tests/test_selenium.py (1)
4-4: LGTM!Also applies to: 19-19, 33-39, 48-48, 63-66, 80-80, 97-97, 137-137
tests/testapp/tests/test_views.py (1)
67-93: LGTM!openwisp_users/admin.py (2)
41-41: LGTM!Also applies to: 102-115, 143-189, 663-669
653-660: 🎯 Functional CorrectnessNo change needed here.
Organization.usersuses an explicit through model, so it isn't exposed as a writable admin many-to-many field.> Likely an incorrect or invalid review comment.openwisp_users/multitenancy.py (1)
23-25: LGTM!Also applies to: 64-123
openwisp_users/tests/test_admin.py (1)
29-29: LGTM!Also applies to: 1868-2160, 2741-2762
tests/testapp/tests/test_multitenancy.py (1)
1-19: LGTM!Also applies to: 83-157
openwisp_users/views.py (1)
36-41: LGTM!openwisp_users/widgets.py (1)
13-13: LGTM!
| default_error_messages = { | ||
| "does_not_exist": _( | ||
| 'Organization with pk "{pk_value}" does not exist or is disabled.' | ||
| ), | ||
| } | ||
|
|
||
| def get_queryset(self): | ||
| user = self.context["request"].user | ||
| if user.is_superuser: | ||
| queryset = Organization.objects.all() | ||
| queryset = Organization.active.all() | ||
| else: | ||
| queryset = Organization.objects.filter(pk__in=user.organizations_managed) | ||
| queryset = Organization.active.filter(pk__in=user.organizations_managed) | ||
| return queryset |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated hardcoded error message across two files. Both sites independently hardcode the identical translatable string 'Organization with pk "{pk_value}" does not exist or is disabled.' for the same disabled-organization condition, risking future wording/translation drift.
openwisp_users/api/serializers.py#L220-L232: extract the string into a shared module-level constant (e.g. inopenwisp_users/api/permissions.pyor a small constants module) and import it here fordefault_error_messages["does_not_exist"].openwisp_users/api/mixins.py#L167-L187: import the same shared constant instead of re-declaring the literal forerror_messages["does_not_exist"].
📍 Affects 2 files
openwisp_users/api/serializers.py#L220-L232(this comment)openwisp_users/api/mixins.py#L167-L187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/api/serializers.py` around lines 220 - 232, The identical
disabled-organization error message is duplicated across serializer and mixin
validation. Define one shared translatable constant in an appropriate API
module, update Organization serializer’s default_error_messages to import and
use it, and update the organization mixin’s error_messages to use the same
constant; apply these changes in openwisp_users/api/serializers.py lines 220-232
and openwisp_users/api/mixins.py lines 167-187.
|
@coderabbitai full review! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_users/api/serializers.py`:
- Around line 410-415: Update the OrganizationUserSerializer membership logic
around org_user.is_admin so is_admin is required and safely validated before
accessing it for an update. Remove the else path that deletes org_user when the
submitted flag is unchanged, preserving the membership while only saving when
the value actually changes.
In `@openwisp_users/tests/test_admin.py`:
- Around line 2027-2037: Update the POST assertion in the relevant admin test to
verify that permission enforcement returns HTTP 403, while retaining the
database-value check if useful. Assert the response status directly so the test
proves has_change_permission blocked the request rather than merely detecting
form validation failure.
In `@openwisp_users/tests/test_models.py`:
- Around line 444-446: In both affected assertions in
openwisp_users/tests/test_models.py (lines 444-446 and 493-496), save
org_user.pk to a local variable before calling org_user.delete(), then query
OrganizationUser using the saved primary key rather than the cleared
org_user.pk. Update both sites consistently while preserving the existing
deletion-count assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e697432e-a81c-4013-8a23-e16569e2ff3f
📒 Files selected for processing (22)
docs/developer/admin-utils.rstdocs/developer/django-rest-framework-utils.rstdocs/user/basic-concepts.rstopenwisp_users/admin.pyopenwisp_users/api/mixins.pyopenwisp_users/api/permissions.pyopenwisp_users/api/serializers.pyopenwisp_users/apps.pyopenwisp_users/base/models.pyopenwisp_users/multitenancy.pyopenwisp_users/tests/test_admin.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_models.pyopenwisp_users/views.pyopenwisp_users/widgets.pytests/testapp/tests/test_admin.pytests/testapp/tests/test_multitenancy.pytests/testapp/tests/test_permission_classes.pytests/testapp/tests/test_selenium.pytests/testapp/tests/test_views.pytests/testapp/urls.pytests/testapp/views.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Place imports at the top of the file; only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside itFor Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_users/widgets.pyopenwisp_users/views.pytests/testapp/urls.pyopenwisp_users/base/models.pyopenwisp_users/apps.pytests/testapp/tests/test_multitenancy.pyopenwisp_users/api/permissions.pytests/testapp/tests/test_admin.pytests/testapp/tests/test_selenium.pyopenwisp_users/tests/test_models.pytests/testapp/views.pyopenwisp_users/multitenancy.pyopenwisp_users/tests/test_admin.pyopenwisp_users/api/mixins.pyopenwisp_users/tests/test_api/test_api.pytests/testapp/tests/test_views.pytests/testapp/tests/test_permission_classes.pyopenwisp_users/api/serializers.pyopenwisp_users/admin.py
🔇 Additional comments (28)
openwisp_users/base/models.py (3)
493-513: Includeuser_idin the disabled-membership no-op comparison.Changing
OrganizationUser.useron a disabled organization still passes as unchanged because onlyorganization_idandis_adminare compared. This remains the previously reported membership-write bypass.
476-479: LGTM!
544-559: LGTM!openwisp_users/multitenancy.py (2)
136-140: Honor the admin write-protection opt-out in the organization queryset.With
disabled_organization_write_protection = False, editing an existing object under a disabled organization still fails because its current organization is excluded from the form field queryset. This remains the previously reported opt-out breakage.
23-25: LGTM!Also applies to: 64-122
docs/developer/admin-utils.rst (1)
46-62: Document the actual opt-out behavior after fixing the form queryset.The documented opt-out is currently ineffective for normal edits of objects already assigned to disabled organizations, because the admin form excludes their submitted organization value.
openwisp_users/apps.py (1)
200-200: LGTM!docs/user/basic-concepts.rst (1)
152-200: LGTM!openwisp_users/api/permissions.py (1)
2-2: LGTM!Also applies to: 98-120
openwisp_users/api/mixins.py (2)
4-16: LGTM!Also applies to: 167-185
322-325: 🔒 Security & PrivacyCreate paths already reject disabled organizations Disabled organizations are filtered out in the serializers used by the list/create views, so
POSTcannot attach them here.DisabledOrgReadOnlyis only needed for update-time object checks.> Likely an incorrect or invalid review comment.docs/developer/django-rest-framework-utils.rst (1)
134-196: LGTM!Also applies to: 314-321
openwisp_users/admin.py (2)
124-140: Previously reported: avoid the per-row organization lookup.
instance.organizationstill risks an extra query for each inline row; the existing review already requestsselect_related("organization").
41-41: LGTM!Also applies to: 102-123, 143-188, 647-669
openwisp_users/api/serializers.py (2)
220-224: Previously reported: share the duplicated disabled-organization error message.This identical translatable message was already flagged across the serializer and API mixin.
7-34: LGTM!Also applies to: 75-176, 226-339, 416-420
openwisp_users/tests/test_models.py (1)
413-443: LGTM!Also applies to: 448-490, 498-524
openwisp_users/tests/test_admin.py (1)
29-29: LGTM!Also applies to: 1868-2020, 2039-2160, 2741-2762
tests/testapp/tests/test_admin.py (1)
50-51: LGTM!tests/testapp/tests/test_multitenancy.py (1)
1-19: LGTM!Also applies to: 83-157
openwisp_users/views.py (1)
36-41: LGTM!tests/testapp/views.py (1)
25-29: LGTM!Also applies to: 216-216, 227-248, 309-312
tests/testapp/urls.py (1)
62-71: LGTM!tests/testapp/tests/test_permission_classes.py (1)
7-7: LGTM!Also applies to: 365-387, 388-405, 406-447, 448-463
openwisp_users/tests/test_api/test_api.py (1)
1-9: LGTM!Also applies to: 24-24, 120-155, 179-222, 661-695, 749-757, 788-820
openwisp_users/widgets.py (1)
13-13: LGTM!tests/testapp/tests/test_views.py (1)
67-92: LGTM!tests/testapp/tests/test_selenium.py (1)
4-4: LGTM!Also applies to: 19-19, 33-39, 48-48, 63-66, 80-80, 97-97, 137-137
| if org_user.is_admin != org_user_data.get("is_admin"): | ||
| org_user.is_admin = org_user_data["is_admin"] | ||
| _full_clean_or_raise(org_user) | ||
| org_user.save() | ||
| else: | ||
| org_user.delete() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline openwisp_users/api/serializers.py \
--items all \
--type class \
--match 'BaseSuperUserSerializer|OrganizationUserSerializer|SuperUserDetailSerializer' \
--view expanded
rg -n -C5 \
'organization_users|is_admin|SuperUserDetailSerializer' \
openwisp_users/api/serializers.py openwisp_users/tests testsRepository: openwisp/openwisp-users
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline openwisp_users/api/serializers.py --view expanded
printf '\n--- relevant lines ---\n'
sed -n '235,430p' openwisp_users/api/serializers.py | cat -nRepository: openwisp/openwisp-users
Length of output: 11232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find tests that exercise nested organization_users payloads and update behavior.
rg -n -C3 'organization_users|is_admin|delete\(\)|update\(self, instance, validated_data\)' \
openwisp_users/tests/test_admin.py openwisp_users/tests/test_api.py openwisp_users/tests \
| head -n 220Repository: openwisp/openwisp-users
Length of output: 17050
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '235,255p' openwisp_users/api/serializers.py | cat -n
printf '\n--- update() block ---\n'
sed -n '396,420p' openwisp_users/api/serializers.py | cat -nRepository: openwisp/openwisp-users
Length of output: 2160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'class OrganizationUser\b|is_admin\b' openwisp_users -g '*.py' | head -n 120Repository: openwisp/openwisp-users
Length of output: 8135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'class OrganizationUser\b|is_admin = models\.BooleanField|default=' openwisp_users/models.py openwisp_users/models -g '*.py'Repository: openwisp/openwisp-users
Length of output: 781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' openwisp_users/models.py | cat -nRepository: openwisp/openwisp-users
Length of output: 2217
🌐 Web query:
Django REST Framework ModelSerializer BooleanField model default required=False omitted validated_data
💡 Result:
In Django REST Framework (DRF), the inclusion of a field in validated_data when it is omitted from the input depends on whether the serializer field has a default value defined [1][2]. If you declare a BooleanField (or any field) with required=False but do not explicitly provide a default value, and the field is omitted from the incoming input, the field will typically be excluded from the validated_data dictionary [1][2]. This means the key will be missing entirely from validated_data, rather than being set to a specific value like False [1][2]. Key behaviors to understand: 1. Omitted Fields: If a field is required=False and has no default, it is not populated in validated_data when omitted [1][2]. This allows you to differentiate between a client explicitly sending False and a client omitting the field. 2. Model Defaults: Note that ModelSerializer does not automatically propagate model-level default values to the serializer field's default keyword argument in all cases [2][3]. If the model field has a default=True, but the serializer field is generated as simply required=False without a default argument, the serializer will not automatically inject that model default into validated_data [2][3]. 3. BooleanField and HTML Inputs: If you are using HTML-encoded form input, DRF treats the omission of a BooleanField as False, regardless of whether a default is specified on the field [1]. This is because HTML checkbox inputs omit the value when unchecked [1]. If you want the field to appear in validated_data with a specific value when omitted, you must explicitly set the default keyword argument on the serializer field definition [1]: class MySerializer(serializers.ModelSerializer): my_boolean = serializers.BooleanField(default=False, required=False) # Will appear in validated_data If you are encountering issues where fields appear as False unexpectedly, ensure that you are not using HTML-encoded form inputs and verify that no default argument is inadvertently set on the field [1]. To ensure consistency with your model's defaults, you can explicitly set the default on your serializer fields [2].
Citations:
- 1: https://github.com/encode/django-rest-framework/blob/main/docs/api-guide/fields.md
- 2: Propagate model field defaults to ModelSerializer fields encode/django-rest-framework#9997
- 3: Missing default value from ModelSerializer encode/django-rest-framework#2683
Require is_admin before updating the membership (openwisp_users/api/serializers.py:410)
OrganizationUserSerializercan omitis_admin, soorg_user_data["is_admin"]can raiseKeyError.- The current
elsebranch also deletes the membership when the flag is unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/api/serializers.py` around lines 410 - 415, Update the
OrganizationUserSerializer membership logic around org_user.is_admin so is_admin
is required and safely validated before accessing it for an update. Remove the
else path that deletes org_user when the submitted flag is unchanged, preserving
the membership while only saving when the value actually changes.
| response = self.client.post( | ||
| change_path, | ||
| { | ||
| "user": str(user.pk), | ||
| "organization": str(org.pk), | ||
| "is_admin": "on", | ||
| }, | ||
| follow=True, | ||
| ) | ||
| org_user.refresh_from_db() | ||
| self.assertFalse(org_user.is_admin) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that permission enforcement returns HTTP 403.
The unchanged database value could also result from form validation failure, so this does not prove has_change_permission blocked the POST.
Proposed fix
response = self.client.post(
change_path,
{
"user": str(user.pk),
"organization": str(org.pk),
"is_admin": "on",
},
follow=True,
)
+ self.assertEqual(response.status_code, 403)
org_user.refresh_from_db()📝 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.
| response = self.client.post( | |
| change_path, | |
| { | |
| "user": str(user.pk), | |
| "organization": str(org.pk), | |
| "is_admin": "on", | |
| }, | |
| follow=True, | |
| ) | |
| org_user.refresh_from_db() | |
| self.assertFalse(org_user.is_admin) | |
| response = self.client.post( | |
| change_path, | |
| { | |
| "user": str(user.pk), | |
| "organization": str(org.pk), | |
| "is_admin": "on", | |
| }, | |
| follow=True, | |
| ) | |
| self.assertEqual(response.status_code, 403) | |
| org_user.refresh_from_db() | |
| self.assertFalse(org_user.is_admin) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/tests/test_admin.py` around lines 2027 - 2037, Update the POST
assertion in the relevant admin test to verify that permission enforcement
returns HTTP 403, while retaining the database-value check if useful. Assert the
response status directly so the test proves has_change_permission blocked the
request rather than merely detecting form validation failure.
| # deleting the row must still work | ||
| org_user.delete() | ||
| self.assertEqual(OrganizationUser.objects.filter(pk=org_user.pk).count(), 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test locations with line numbers.
sed -n '420,510p' openwisp_users/tests/test_models.py
# Search for nearby delete/assert patterns in the same file.
rg -n "delete\(\)|filter\(pk=.*count\(\)" openwisp_users/tests/test_models.pyRepository: openwisp/openwisp-users
Length of output: 5608
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant model definitions.
rg -n "class OrganizationUser|class OrganizationOwner|def delete\(" openwisp_users -g '*.py'
# Show the model implementations around any delete overrides if present.
python3 - <<'PY'
from pathlib import Path
paths = [Path("openwisp_users/models.py")]
for p in paths:
if p.exists():
text = p.read_text()
for needle in ["class OrganizationUser", "class OrganizationOwner", "def delete("]:
if needle in text:
print(f"\n=== {p} contains {needle} ===")
# print surrounding chunks
idx = 0
while True:
idx = text.find(needle, idx)
if idx == -1:
break
start = max(0, text.rfind("\n", 0, idx-800))
end = min(len(text), text.find("\n\n", idx+1))
print(text[start:end])
idx += len(needle)
PYRepository: openwisp/openwisp-users
Length of output: 2246
Capture the primary key before calling delete(). Model.delete() clears pk, so both assertions currently query pk=None instead of the deleted rows.
openwisp_users/tests/test_models.py#L444-L446openwisp_users/tests/test_models.py#L493-L496
📍 Affects 1 file
openwisp_users/tests/test_models.py#L444-L446(this comment)openwisp_users/tests/test_models.py#L493-L496
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/tests/test_models.py` around lines 444 - 446, In both affected
assertions in openwisp_users/tests/test_models.py (lines 444-446 and 493-496),
save org_user.pk to a local variable before calling org_user.delete(), then
query OrganizationUser using the saved primary key rather than the cleared
org_user.pk. Update both sites consistently while preserving the existing
deletion-count assertions.
| org1.is_active = False | ||
| org1.save() | ||
| path = reverse("users:organization_detail", args=(org1.pk,)) | ||
| # re-enabling and editing another field in one request is rejected, |
There was a problem hiding this comment.
This should be present in the docstring.
| with mock.patch.object( | ||
| OrganizationUser, | ||
| "full_clean", | ||
| side_effect=DjangoValidationError("membership boom"), | ||
| ): |
There was a problem hiding this comment.
Instead of using side_effect, a more meaningful test would be to use a disabled organization. Will change the behaviour of the test?
Moreover, we need to test this in a TransactionTestCase.
| class LibraryParentAdmin(MultitenantAdminMixin, admin.ModelAdmin): | ||
| # Library has no organization field; it is reached through its Book parent | ||
| multitenant_parent = "book" |
There was a problem hiding this comment.
Are you sure this is the right place for placing the ModelAdmin? Is this only for testing?
I'd prefer to move it to the admin.py file, so we can also test this manually.
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_users/api/serializers.py (1)
115-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA genuinely no-op request on a disabled organization is rejected instead of accepted.
allowedrequires(reenabling or is_owner_unassignment)even whenchanged_keysis empty. An empty PATCH body ({}) or a PATCH that only re-sends the currentis_active: Falseunchanged produceschanged_keys == set()but still fails this condition, raising 400 for a request that changes nothing.🔧 Proposed fix
allowed = ( - changed_keys <= {"is_active", "owner"} + not changed_keys + or changed_keys <= {"is_active", "owner"} and (not owner_present or is_owner_unassignment) and (reenabling or is_owner_unassignment) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_users/api/serializers.py` around lines 115 - 152, The disabled-organization validation in validate must accept genuine no-op requests. Update the allowed-condition logic so an empty changed_keys set is valid, while preserving the existing restrictions for actual changes: only re-enabling and/or owner unassignment, with no other edits or owner assignment.openwisp_users/multitenancy.py (1)
93-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow
multitenant_parentmodels to add shared parents
has_add_permission()hides Add for any non-superuser with no managed orgs, including models that only usemultitenant_parent. Those forms can still submit against shared (organization=None) parents, so this blocks valid creates. Restrict the guard to models with a directorganizationfield.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_users/multitenancy.py` around lines 93 - 115, Update has_add_permission so the no-managed-organizations guard only returns False when self.model has a direct organization field. Remove the multitenant_parent condition from this check, preserving the existing superuser and User exclusions and allowing shared parent models to be added.
♻️ Duplicate comments (1)
openwisp_users/api/serializers.py (1)
406-430: 🩺 Stability & Availability | 🟠 Major
KeyErrorrisk whenis_adminis omitted from the membership payload.
org_user.is_admin != org_user_data.get("is_admin")uses a safe.get(), but the following assignmentorg_user.is_admin = org_user_data["is_admin"](Line 421) indexes the dict directly. Ifis_adminis omitted from the request (DRF's auto-generated field for a model field with a default typically won't inject adefaultintovalidated_datawhen omitted),.get()returnsNone, which mismatches an existingTrue/Falsevalue and triggers this branch — raising an uncaughtKeyError(500) instead of a clean 400. This mirrors a previously flagged concern on this exact update path that appears unresolved after the restructuring.🐛 Proposed fix (clarify intended semantics for omitted `is_admin`)
if org_user: - if org_user.is_admin != org_user_data.get("is_admin"): - org_user.is_admin = org_user_data["is_admin"] + is_admin = org_user_data.get("is_admin", org_user.is_admin) + if org_user.is_admin != is_admin: + org_user.is_admin = is_admin _full_clean_or_raise(org_user) org_user.save() else: org_user.delete()Note: with this fix, omitting
is_adminnow falls through to the "unchanged" branch and deletes the membership — confirm that's the intended contract for a submission that only specifiesorganizationwithoutis_admin.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_users/api/serializers.py` around lines 406 - 430, Update the existing-membership branch in update so omitted is_admin values cannot cause a KeyError: distinguish a missing key from an explicitly supplied value and only assign/save when is_admin is present, while preserving the existing deletion behavior for the unchanged branch. Confirm and retain the intended contract that an organization-only membership payload deletes an existing membership.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openwisp_users/api/serializers.py`:
- Around line 115-152: The disabled-organization validation in validate must
accept genuine no-op requests. Update the allowed-condition logic so an empty
changed_keys set is valid, while preserving the existing restrictions for actual
changes: only re-enabling and/or owner unassignment, with no other edits or
owner assignment.
In `@openwisp_users/multitenancy.py`:
- Around line 93-115: Update has_add_permission so the no-managed-organizations
guard only returns False when self.model has a direct organization field. Remove
the multitenant_parent condition from this check, preserving the existing
superuser and User exclusions and allowing shared parent models to be added.
---
Duplicate comments:
In `@openwisp_users/api/serializers.py`:
- Around line 406-430: Update the existing-membership branch in update so
omitted is_admin values cannot cause a KeyError: distinguish a missing key from
an explicitly supplied value and only assign/save when is_admin is present,
while preserving the existing deletion behavior for the unchanged branch.
Confirm and retain the intended contract that an organization-only membership
payload deletes an existing membership.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7aa1685b-087f-4523-ae5b-6621ac92f5d3
📒 Files selected for processing (9)
openwisp_users/admin.pyopenwisp_users/api/mixins.pyopenwisp_users/api/serializers.pyopenwisp_users/base/models.pyopenwisp_users/multitenancy.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_models.pytests/testapp/admin.pytests/testapp/tests/test_multitenancy.py
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
8498641 to
7aeb9f1
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_users/base/models.py (1)
465-479: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winForward
**kwargswhen constructing the membership.
add_user()still advertises extension fields through**kwargs, but silently drops them. CustomOrganizationUsermodels relying on these values may fail validation or persist incomplete data.Proposed fix
- org_user = OrganizationUser(user=user, organization=self, is_admin=is_admin) + org_user = OrganizationUser( + user=user, + organization=self, + is_admin=is_admin, + **kwargs, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_users/base/models.py` around lines 465 - 479, Update Organization.add_user to forward the received **kwargs when constructing the OrganizationUser membership, preserving the existing user, organization, and is_admin values while allowing custom membership fields to validate and persist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/basic-concepts.rst`:
- Around line 162-183: Update the opening statement in the disabled-organization
documentation to clarify that its data remains readable and deletable only by
users who retain the relevant permissions, notably superusers. Keep the existing
restrictions on managers and other users consistent with the later access
description.
In `@openwisp_users/api/permissions.py`:
- Around line 115-120: Update the organization resolution logic around
get_object_organization so AttributeError from a misconfigured
organization_field or invalid traversal is not treated as a non-organizational
object. Only allow the explicit non-organizational opt-out to bypass the check;
otherwise propagate the configuration error or deny access rather than returning
True.
In `@openwisp_users/api/serializers.py`:
- Around line 165-169: Update both owner-creation paths in the relevant
serializer to instantiate OrganizationOwner without persisting it, call
_full_clean_or_raise() on the instance, then invoke save() only after validation
succeeds. Preserve the existing organization and organization_user assignments
and apply the same ordering consistently in both paths.
In `@openwisp_users/base/models.py`:
- Around line 493-514: Update clean() at openwisp_users/base/models.py:493-514
to reject membership changes when either the persisted organization or submitted
organization is disabled, while preserving no-op saves for existing memberships.
Apply the same persisted-source organization check to the owner-change
validation at openwisp_users/base/models.py:544-560.
In `@openwisp_users/multitenancy.py`:
- Around line 80-91: Separate active-organization write filtering from
read/delete scoping: in openwisp_users/multitenancy.py lines 80-91, update the
admin queryset/permission flow so disabled-organization objects remain readable
and deletable while add/edit choices remain active-only; use the existing
symbols around has_change_permission and organization scoping. In
openwisp_users/api/mixins.py lines 327-330, allow safe methods and DELETE to
resolve memberships and objects from disabled organizations, while preserving
active-only filtering for create/update inputs.
In `@openwisp_users/tests/test_api/test_api.py`:
- Around line 961-980: Update
test_create_user_organization_users_disabled_org_api to use an active
organization when posting the request, then deactivate that organization
immediately before the serializer save occurs so validation succeeds and
SuperUserListSerializer.create reaches the rollback path. Preserve the
assertions that the response is 400 and neither the User nor OrganizationUser
record remains.
---
Outside diff comments:
In `@openwisp_users/base/models.py`:
- Around line 465-479: Update Organization.add_user to forward the received
**kwargs when constructing the OrganizationUser membership, preserving the
existing user, organization, and is_admin values while allowing custom
membership fields to validate and persist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 981d61ab-3e2c-4503-aff9-7cf964a0bca9
📒 Files selected for processing (23)
docs/developer/admin-utils.rstdocs/developer/django-rest-framework-utils.rstdocs/user/basic-concepts.rstopenwisp_users/admin.pyopenwisp_users/api/mixins.pyopenwisp_users/api/permissions.pyopenwisp_users/api/serializers.pyopenwisp_users/apps.pyopenwisp_users/base/models.pyopenwisp_users/multitenancy.pyopenwisp_users/tests/test_admin.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/tests/test_models.pyopenwisp_users/views.pyopenwisp_users/widgets.pytests/testapp/admin.pytests/testapp/tests/test_admin.pytests/testapp/tests/test_multitenancy.pytests/testapp/tests/test_permission_classes.pytests/testapp/tests/test_selenium.pytests/testapp/tests/test_views.pytests/testapp/urls.pytests/testapp/views.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Place imports at the top of the file; only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside itFor Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
tests/testapp/tests/test_admin.pytests/testapp/urls.pyopenwisp_users/apps.pytests/testapp/tests/test_views.pyopenwisp_users/widgets.pytests/testapp/admin.pyopenwisp_users/views.pyopenwisp_users/base/models.pytests/testapp/tests/test_selenium.pytests/testapp/tests/test_permission_classes.pyopenwisp_users/tests/test_models.pyopenwisp_users/tests/test_admin.pytests/testapp/tests/test_multitenancy.pyopenwisp_users/api/serializers.pyopenwisp_users/tests/test_api/test_api.pyopenwisp_users/api/permissions.pyopenwisp_users/api/mixins.pyopenwisp_users/multitenancy.pytests/testapp/views.pyopenwisp_users/admin.py
🪛 ast-grep (0.44.1)
openwisp_users/api/serializers.py
[warning] 17-17: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🔇 Additional comments (26)
openwisp_users/tests/test_admin.py (2)
2027-2037: Assert that the POST is rejected with HTTP 403.The unchanged database value alone does not prove permission enforcement rather than form validation failure. This concern was already raised in the prior review.
29-29: LGTM!Also applies to: 1868-2006, 2039-2160, 2741-2762
openwisp_users/apps.py (1)
198-201: LGTM!tests/testapp/tests/test_admin.py (1)
49-51: LGTM!tests/testapp/tests/test_views.py (1)
67-93: LGTM!tests/testapp/tests/test_selenium.py (1)
4-6: LGTM!Also applies to: 20-20, 29-52, 61-81, 95-95, 112-112, 152-152
docs/developer/admin-utils.rst (1)
34-66: LGTM!docs/developer/django-rest-framework-utils.rst (1)
134-196: LGTM!Also applies to: 314-322
openwisp_users/api/serializers.py (2)
420-425: The existing membership can still be deleted whenis_adminis unchanged, and an omittedis_admincan still raiseKeyError.This remains covered by the previous review comment.
7-36: LGTM!Also applies to: 85-85, 115-153, 232-241, 332-349
openwisp_users/tests/test_models.py (2)
444-446: Both deletion assertions still query with a primary key cleared bydelete().Save each primary key before deleting and query with the saved value. This was already raised in the previous review.
Also applies to: 508-511
413-443: LGTM!Also applies to: 447-507, 512-539
openwisp_users/api/permissions.py (1)
2-2: LGTM!Also applies to: 98-114
tests/testapp/urls.py (1)
62-71: LGTM!tests/testapp/tests/test_permission_classes.py (1)
7-7: LGTM!Also applies to: 365-463
openwisp_users/tests/test_api/test_api.py (1)
7-21: LGTM!Also applies to: 117-245, 737-808
docs/user/basic-concepts.rst (1)
152-161: LGTM!Also applies to: 185-200
openwisp_users/multitenancy.py (1)
23-78: LGTM!Also applies to: 93-170
openwisp_users/admin.py (2)
41-41: LGTM!Also applies to: 102-195, 669-674
653-667: 🎯 Functional Correctness
OrganizationAdmindoesn’t needlocal_many_to_manyhere. The disabled-org readonly rule only needs the editable fields already exposed on this form; no local many-to-many field is present.> Likely an incorrect or invalid review comment.tests/testapp/admin.py (1)
71-79: LGTM!tests/testapp/tests/test_multitenancy.py (1)
1-22: LGTM!Also applies to: 87-187
openwisp_users/api/mixins.py (1)
4-23: LGTM!Also applies to: 169-203
tests/testapp/views.py (1)
25-29: LGTM!Also applies to: 210-248, 309-312
openwisp_users/views.py (1)
31-42: LGTM!openwisp_users/widgets.py (1)
12-13: LGTM!
| Disabling an organization does not delete anything: all of its data, | ||
| including users, memberships, and related objects, remains fully | ||
| **readable** and **deletable**. What changes is: | ||
|
|
||
| - **No new object can be created for a disabled organization**, and | ||
| **existing objects belonging to it cannot be modified**, superusers | ||
| included. This applies to the organization's own record too: once | ||
| disabled, only its **Is active** flag can be changed (to re-enable it) | ||
| or its owner unassigned; everything else is locked until it is | ||
| re-enabled. | ||
| - Deleting objects, including the organization itself, is always allowed, | ||
| so cleanup is never blocked. | ||
| - The organization stops appearing in **organization selection widgets** | ||
| (e.g. when creating a new object), so it can no longer be picked for new | ||
| data. It still appears in admin **list filters**, so its existing data | ||
| remains easy to find for auditing purposes. | ||
| - Re-enabling a disabled organization is allowed **only for superusers**. | ||
| Once an organization is disabled, its managers lose access to it (a | ||
| disabled organization is no longer part of the organizations they | ||
| manage), so they can no longer edit it, including re-enabling it. A | ||
| superuser must re-enable the organization before its managers regain | ||
| access. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify who can still read and delete disabled-organization data.
“Fully readable and deletable” conflicts with the later statement that managers lose access. State that data remains available to users who retain permission, notably superusers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/basic-concepts.rst` around lines 162 - 183, Update the opening
statement in the disabled-organization documentation to clarify that its data
remains readable and deletable only by users who retain the relevant
permissions, notably superusers. Keep the existing restrictions on managers and
other users consistent with the later access description.
| try: | ||
| organization = self.get_object_organization(view, obj) | ||
| except AttributeError: | ||
| # object has no organization field, rule not applicable | ||
| return True | ||
| return organization is None or organization.is_active |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not fail open when organization resolution is misconfigured.
Catching every AttributeError allows writes if organization_field is misspelled or its traversal is invalid. Fail closed or require non-organizational views to use the explicit opt-out.
try:
organization = self.get_object_organization(view, obj)
except AttributeError:
- # object has no organization field, rule not applicable
- return True
+ return False📝 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.
| try: | |
| organization = self.get_object_organization(view, obj) | |
| except AttributeError: | |
| # object has no organization field, rule not applicable | |
| return True | |
| return organization is None or organization.is_active | |
| try: | |
| organization = self.get_object_organization(view, obj) | |
| except AttributeError: | |
| return False | |
| return organization is None or organization.is_active |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/api/permissions.py` around lines 115 - 120, Update the
organization resolution logic around get_object_organization so AttributeError
from a misconfigured organization_field or invalid traversal is not treated as a
non-organizational object. Only allow the explicit non-organizational opt-out to
bypass the check; otherwise propagate the configuration error or deny access
rather than returning True.
| org_owner = OrganizationOwner.objects.create( | ||
| organization=instance, organization_user=org_user | ||
| ) | ||
| org_owner.full_clean() | ||
| _full_clean_or_raise(org_owner) | ||
| org_owner.save() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'post_save|`@receiver`\s*\(' openwisp_users -g '*.py'Repository: openwisp/openwisp-users
Length of output: 4150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the serializer around both owner-creation paths
sed -n '140,210p' openwisp_users/api/serializers.py | cat -n
# Inspect signal hookups and handlers related to owner/user creation
sed -n '1,240p' openwisp_users/apps.py | cat -n
# Find any post_save / pre_save handlers that might be triggered by OrganizationOwner or OrganizationUser
rg -n -C3 'OrganizationOwner|OrganizationUser|post_save|pre_save|`@receiver`\s*\(' openwisp_users -g '*.py'Repository: openwisp/openwisp-users
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
serializer = Path("openwisp_users/api/serializers.py").read_text()
apps = Path("openwisp_users/apps.py").read_text()
# Show the two owner-creation sequences in the serializer.
for pat in [
r"OrganizationOwner\.objects\.create\([\s\S]*?_full_clean_or_raise\(org_owner\)[\s\S]*?org_owner\.save\(\)",
r"OrganizationOwner\.objects\.create\([\s\S]*?_full_clean_or_raise\(org_owner\)[\s\S]*?org_owner\.save\(\)",
]:
m = re.search(pat, serializer)
print("serializer_match:", bool(m))
# Show save-signal wiring for OrganizationOwner.
print("OrganizationOwner post_save connected:", "post_save.connect(" in apps and "sender=model" in apps and "OrganizationOwner" in apps)
print("OrganizationOwner pre_save connected:", "pre_save.connect(" in apps and "OrganizationOwner" in apps)
# Extract the receiver used for OrganizationOwner/OrganizationUser saves.
for line in apps.splitlines():
if "update_organizations_dict" in line or "pre_save_update_organizations_dict" in line or "create_organization_owner" in line:
print(line)
PYRepository: openwisp/openwisp-users
Length of output: 728
Validate owner objects before saving. objects.create() triggers OrganizationOwner save receivers, and update_organizations_dict repopulates the user's cache from the uncommitted row; if full_clean() raises afterward, the DB rolls back but the cache can still reflect the invalid owner. Build the instance, run _full_clean_or_raise(), then save it in both owner-creation paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/api/serializers.py` around lines 165 - 169, Update both
owner-creation paths in the relevant serializer to instantiate OrganizationOwner
without persisting it, call _full_clean_or_raise() on the instance, then invoke
save() only after validation succeeds. Preserve the existing organization and
organization_user assignments and apply the same ordering consistently in both
paths.
| def clean(self): | ||
| if self.organization_id and not self.organization.is_active: | ||
| if self._state.adding: | ||
| raise ValidationError( | ||
| {"organization": _("Cannot add users to a disabled organization.")} | ||
| ) | ||
| # Only block real modifications: Django re-runs full_clean() on | ||
| # untouched inline rows, so a no-op save of a user who belongs to a | ||
| # disabled organization must not fail. | ||
| db_values = ( | ||
| self._meta.model.objects.filter(pk=self.pk) | ||
| .values("organization_id", "is_admin", "user_id") | ||
| .first() | ||
| ) | ||
| if db_values is None or ( | ||
| db_values["organization_id"] != self.organization_id | ||
| or db_values["is_admin"] != self.is_admin | ||
| or db_values["user_id"] != self.user_id | ||
| ): | ||
| raise ValidationError( | ||
| _("Memberships of a disabled organization cannot be modified.") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check both the persisted and submitted organizations.
Both guards can be bypassed by moving a row from a disabled organization to an active one.
openwisp_users/base/models.py#L493-L514: when membership fields change, reject the update if either the persisted or submitted organization is disabled.openwisp_users/base/models.py#L544-L560: apply the same persisted-source check to owner changes.
📍 Affects 1 file
openwisp_users/base/models.py#L493-L514(this comment)openwisp_users/base/models.py#L544-L560
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/base/models.py` around lines 493 - 514, Update clean() at
openwisp_users/base/models.py:493-514 to reject membership changes when either
the persisted organization or submitted organization is disabled, while
preserving no-op saves for existing memberships. Apply the same persisted-source
organization check to the owner-change validation at
openwisp_users/base/models.py:544-560.
| def has_change_permission(self, request, obj=None): | ||
| """ | ||
| Objects belonging to a disabled organization stay readable and | ||
| deletable, but cannot be changed, regardless of the user being a | ||
| superuser. Subclasses can opt out with | ||
| ``disabled_organization_write_protection = False``. | ||
| """ | ||
| if self.disabled_organization_write_protection and obj is not None: | ||
| organization = self._get_object_organization(obj) | ||
| if organization is not None and not organization.is_active: | ||
| return False | ||
| return super().has_change_permission(request, obj) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Separate read/delete organization scope from active write scope.
Both guards run after earlier authorization layers have already excluded disabled organizations, so non-superuser managers cannot view or delete their objects.
openwisp_users/multitenancy.py#L80-L91: include disabled organizations in admin read/delete queryset scoping; keep active-only organizations for add and edit choices.openwisp_users/api/mixins.py#L327-L330: allow safe methods and DELETE to resolve memberships and objects from disabled organizations; retain active-only filtering for create/update inputs.
📍 Affects 2 files
openwisp_users/multitenancy.py#L80-L91(this comment)openwisp_users/api/mixins.py#L327-L330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/multitenancy.py` around lines 80 - 91, Separate
active-organization write filtering from read/delete scoping: in
openwisp_users/multitenancy.py lines 80-91, update the admin queryset/permission
flow so disabled-organization objects remain readable and deletable while
add/edit choices remain active-only; use the existing symbols around
has_change_permission and organization scoping. In openwisp_users/api/mixins.py
lines 327-330, allow safe methods and DELETE to resolve memberships and objects
from disabled organizations, while preserving active-only filtering for
create/update inputs.
| class TestUsersApiTransaction(TestOrganizationMixin, TransactionTestCase): | ||
| def setUp(self): | ||
| self.client.force_login(self._get_admin()) | ||
|
|
||
| def test_create_user_organization_users_disabled_org_api(self): | ||
| # A membership validation failure (here, the disabled-organization | ||
| # guard) after the user row is written must roll the user back | ||
| # instead of leaving a half-created account behind. | ||
| path = reverse("users:user_list") | ||
| org1 = self._create_org(name="disabled-org", is_active=False) | ||
| data = { | ||
| "username": "tester", | ||
| "email": "tester@test.com", | ||
| "password": "password", | ||
| "organization_users": {"is_admin": False, "organization": org1.pk}, | ||
| } | ||
| r = self.client.post(path, data, content_type="application/json") | ||
| self.assertEqual(r.status_code, 400) | ||
| self.assertEqual(User.objects.filter(username="tester").count(), 0) | ||
| self.assertEqual(OrganizationUser.objects.filter(organization=org1).count(), 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline openwisp_users/api/serializers.py \
--items all --match 'OrgUserCustomPrimarykeyRelatedField|SuperUserListSerializer'
rg -n -C4 \
'OrgUserCustomPrimarykeyRelatedField|organization_users\s*=' \
openwisp_users/api/serializers.pyRepository: openwisp/openwisp-users
Length of output: 1832
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '231,340p' openwisp_users/api/serializers.py | cat -n
printf '\n---\n'
sed -n '1,220p' openwisp_users/tests/test_api/test_api.py | cat -n | sed -n '900,1020p'Repository: openwisp/openwisp-users
Length of output: 4525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant serializer and test lines with context.
sed -n '231,330p' openwisp_users/api/serializers.py | cat -n
printf '\n--- TEST ---\n'
sed -n '950,990p' openwisp_users/tests/test_api/test_api.py | cat -nRepository: openwisp/openwisp-users
Length of output: 5710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the create/validation logic for organization_users.
rg -n -C8 'def validate_email|def create|organization_users|OrgUserCustomPrimarykeyRelatedField|get_queryset' openwisp_users/api/serializers.py
# Show the test case around the cited lines.
sed -n '955,985p' openwisp_users/tests/test_api/test_api.py | cat -nRepository: openwisp/openwisp-users
Length of output: 8070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba openwisp_users/api/serializers.py | sed -n '231,330p'Repository: openwisp/openwisp-users
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '326,390p' openwisp_users/api/serializers.py | cat -nRepository: openwisp/openwisp-users
Length of output: 2954
Use an active org to hit the rollback path
OrgUserCustomPrimarykeyRelatedField.get_queryset() filters out inactive orgs during is_valid(), so this request fails before SuperUserListSerializer.create() writes the user. Create with an active org, then deactivate it before save() to exercise the rollback branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_users/tests/test_api/test_api.py` around lines 961 - 980, Update
test_create_user_organization_users_disabled_org_api to use an active
organization when posting the request, then deactivate that organization
immediately before the serializer save occurs so validation succeeds and
SuperUserListSerializer.create reaches the rollback path. Preserve the
assertions that the response is 400 and neither the User nor OrganizationUser
record remains.
3 participants
Checklist
Reference to Existing Issue
Closes #522
Description of Changes
Made disabled organizations read-only but deletable: objects belonging
to a disabled organization can still be viewed and deleted, but not
created or modified, in both the admin interface and the REST API. This
also applies to the organization record itself (only re-enabling or
unassigning its owner is allowed while disabled). Organization selection
widgets exclude disabled organizations, while admin list filters keep
them visible for auditing.