[feature] Made disabled organizations readonly but deletable #522 - #542
[feature] Made disabled organizations readonly but deletable #522#542pandafy wants to merge 18 commits into
Conversation
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 (2 files)
Previous Review Summaries (10 snapshots, latest commit 12af5e7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 12af5e7)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 0434cf8)Status: No Issues Found | Recommendation: Merge Files Reviewed (18 files)
Previous review (commit 5b70bce)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (14 files)
Fix these issues in Kilo Cloud Previous review (commit 62a3169)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit f1c67c9)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit b73b5b2)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit b11a450)Status: No Issues Found | Recommendation: Merge Files Reviewed (14 files)
Previous review (commit 1c8a478)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous review (commit b1f968a)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
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: 60K · Output: 14.1K · Cached: 71.7K |
|
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!
|
@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.
| 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!
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Good progress but incomplete, requires more work and more testing with other key modules.
| if db_field.name == "organization" and db_field.name in ( | ||
| self.get_autocomplete_fields(request) | ||
| ): | ||
| kwargs["widget"] = OrganizationAutocompleteSelect( |
There was a problem hiding this comment.
In manual testing, the user add page freezes the browser on this branch. The new organization autocomplete is now used by this inline, but org-autocomplete.js assumes there is a select#id_organization on the page. Inline fields use ids like id_openwisp_users_organizationuser-0-organization, so the jQuery selection is empty and the parent-walking loop never reaches a form. Please check the autocomplete initialization for inline usage. A minimal fix could be to skip the script when the expected field is missing, or make it target the actual widget instance instead of a hardcoded id.
| via the inline's delete action, which does not go through here). | ||
| """ | ||
| fields = super().get_readonly_fields(request, obj) | ||
| if obj and not obj.is_active: |
There was a problem hiding this comment.
In manual testing, disabling an organization makes the main organization fields read-only, but downstream inlines such as config management settings, organization variables, and geo settings remain writable. Please look into whether this can be handled centrally for organization admin inlines, or through the multitenancy admin utilities, instead of requiring each downstream inline to implement the same guard. Inline writes for disabled organizations need test coverage because they are another path for modifying disabled-organization data.
| abstract = True | ||
|
|
||
| def clean(self): | ||
| if self.organization_id and not self.organization.is_active: |
There was a problem hiding this comment.
This guard only checks the submitted organization. If an existing membership belongs to a disabled organization, it can be moved to an active organization and full_clean() will not block it. The owner guard below has the same issue. Please reject real changes when either the persisted organization or the submitted organization is disabled, while still allowing no-op validation and deletion.
| return True | ||
| try: | ||
| organization = self.get_object_organization(view, obj) | ||
| except AttributeError: |
There was a problem hiding this comment.
This fails open if organization_field is misspelled or the traversal is broken. I would not treat that as a non-organizational object. Either deny access or let the configuration error propagate. Views that intentionally are not organization-bound should opt out explicitly instead of relying on this exception path.
| @@ -110,7 +165,7 @@ def update(self, instance, validated_data): | |||
| org_owner = OrganizationOwner.objects.create( | |||
There was a problem hiding this comment.
This saves OrganizationOwner before validation. Save receivers run immediately and can repopulate the user's organization cache from a row that can still be rejected and rolled back by _full_clean_or_raise(). Please build the instance, validate it, then save it. The same applies to the other owner creation path below.
| """ | ||
| 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: |
There was a problem hiding this comment.
This new write guard runs after queryset scoping has already filtered disabled organizations out for non-superusers through user.organizations_managed, which only includes active organizations. As a result, an organization manager who disables an organization loses the ability to see or delete its existing objects. If the intended behavior is superuser-only access after disable, the issue and docs should say so. Otherwise, read and delete scoping needs to include disabled organizations while write choices remain active-only.
| org_user.save() | ||
| else: | ||
| org_user.delete() | ||
| if org_user.is_admin != org_user_data.get("is_admin"): |
There was a problem hiding this comment.
This path can delete an existing membership when is_admin is submitted unchanged, and it can raise KeyError if is_admin is omitted but .get("is_admin") differs from the current value. Please make the contract explicit: require is_admin, or treat omission as leave unchanged. In either case, do not delete the membership implicitly unless that behavior is intentional and covered by a test.
| # 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) |
There was a problem hiding this comment.
This test does not exercise the rollback path it describes. OrgUserCustomPrimarykeyRelatedField.get_queryset() only returns active organizations, so this request fails during serializer validation before SuperUserListSerializer.create() writes the user. Use an active organization for is_valid(), then deactivate it before save() so the membership validation fails after the user row has been written inside the transaction.
| ): | ||
| org_user.full_clean() | ||
| # deleting the row must still work | ||
| org_user.delete() |
There was a problem hiding this comment.
Please store the primary key before calling delete(). Django clears pk on delete, so this assertion queries pk=None and does not prove this row was removed. The owner deletion assertion below has the same issue.
|
Follow-up from manual testing in I tested admin objects that belong to a disabled organization. Device admin still has issues:
Other objects belonging to the same disabled organization looked ok in the same test:
The reference to If a module-specific admin implementation is bypassing this protection, we should identify the pattern and standardize it as much as possible, so downstream modules do not need to reimplement disabled-organization protection differently. |
|
Hi @pandafy 👋, This is a friendly reminder that this pull request has had no activity for 7 days since changes were requested. We'd love to see this contribution merged! Please take a moment to:
If you're busy or need more time, no worries! Just leave a comment to let us know you're still working on it. Note: within 7 more days, the linked issue will be unassigned to allow other contributors to work on it. Thank you for your contribution! 🙏 |
|
Hi @pandafy 👋, This pull request has been marked as stale due to 14 days of inactivity after changes were requested. As a result, any linked issues are being unassigned from you so other contributors can pick them up. However, you can still continue working on this PR! If you push new commits or respond to the review feedback:
If you need more time or have questions about the requested changes, please let us know. We're happy to help! 🤝 |
6fe87ca to
f1c67c9
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| content_type="application/json", | ||
| HTTP_AUTHORIZATION=f"Bearer {token}", | ||
| ) | ||
| with self.assertRaises(FieldError): |
There was a problem hiding this comment.
CRITICAL: assertRaises(FieldError) is incorrect — DRF's dispatch() catches all exceptions (including FieldError from select_related) and converts them to HTTP responses, so FieldError is never propagated to the test caller. The original test correctly checked response.status_code == 403. The test should verify the 403 response instead.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
AI-assisted review using GPT-5.6 Sol high. I read the PR description, all review and issue comments, issue #522 and its linked OpenWISP follow-ups, and the complete current diff. I found several remaining gaps in the reusable guards.
One integration issue cannot be placed inline because it is in a downstream repository: openwisp_controller/config/admin.py:603-607 calls super().has_change_permission(request) without forwarding obj, so MultitenantAdminMixin receives obj=None and direct Device edits remain writable after the organization is disabled. I do not expect controller-specific code in this PR, but I need that controller fix and regression coverage before treating the reusable guard as validated.
All current GitHub checks pass, including the full Python and Django matrix. The passing tests do not cover the gaps below, and one test currently codifies the HTTP 500 described inline.
The AGENTS.md:13-55 contributor-policy rewrite is unrelated to issue #522. Please drop those changes from this branch and handle them separately so this already large security and multi-tenancy change stays focused.
| def get_queryset(self): | ||
| qs = super().get_queryset() | ||
| if getattr(self, "select_related_organization", True): | ||
| qs = qs.select_related(self.org_field) |
There was a problem hiding this comment.
select_related(self.org_field) runs before object permissions. A misspelled organization_field raises FieldError while evaluating the queryset, so DisabledOrgReadOnly never gets to return False and the request ends as HTTP 500. The test at tests/testapp/tests/test_permission_classes.py:630-647 currently asserts this crash. Please validate the relation before applying this optimization, or avoid it when invalid, then assert 403 and an unchanged object while preserving select_related() for valid direct and nested paths.
| superuser. Subclasses can opt out with | ||
| ``disabled_organization_write_protection = False``. | ||
| """ | ||
| if self.disabled_organization_write_protection and obj is not None: |
There was a problem hiding this comment.
This protects object change views, but mutating changelist actions call has_change_permission(request, obj=None) and then receive the selected queryset. A superuser can therefore run an update action on disabled-organization rows. openwisp_controller/config/admin.py:722-749 is a concrete example because change_group calls queryset.update(). Please reject or remove disabled-organization rows before non-delete actions execute, preserve delete actions and the opt-out, and add an action regression test.
| allowed = Q(is_active=True) | ||
| if keep_disabled_org_pk is not None: | ||
| allowed |= Q(pk=keep_disabled_org_pk) | ||
| org_field.queryset = org_field.queryset.filter(allowed) |
There was a problem hiding this comment.
This relation filtering is inside the non-superuser branch, so superusers can still select a shelf, template, CA, VPN, credentials object, or another relation belonging to a disabled organization. Issue #522 explicitly requires other FK pickers to exclude disabled organizations for everyone. Please always restrict these querysets to active organizations or shared objects, then apply the managed-organization restriction for non-superusers. The existing superuser expectation in test_book_shelf_fk_queryset also needs to change.
| user = self.context["request"].user | ||
| if user.is_superuser: | ||
| queryset = Organization.objects.all() | ||
| queryset = Organization.active.all() |
There was a problem hiding this comment.
Filtering every disabled organization here also prevents deletion of an existing disabled-organization membership. Field validation returns 400 before SuperUserDetailSerializer.update() can resolve the membership and reach its deletion branch, and there is no other REST deletion path for it. Please allow only the current user's existing disabled membership to resolve on detail updates. New disabled memberships must remain invalid and role changes must remain blocked. Add a regression test that deletes the existing membership.
| pk=instance.organization_id | ||
| ) | ||
| if "is_admin" in form.fields: | ||
| form.fields["is_admin"].disabled = True |
There was a problem hiding this comment.
This disables only the two built-in membership fields. If a swapped OrganizationUser adds a field and a downstream inline exposes it, that field remains writable; BaseOrganizationUser.clean() also compares only the built-in fields. I would derive the protected fields from model or form metadata and cover an extended membership model.
Confidence: medium. This requires a downstream swapped model exposing an additional membership field, but swapped-model support is a documented project requirement.
| for f in self.model._meta.local_fields | ||
| if f.editable and f.name != "is_active" | ||
| ] | ||
| fields = list(fields) + [f for f in editable_fields if f not in fields] |
There was a problem hiding this comment.
local_fields omits editable many-to-many fields added by a swapped Organization model, so those fields remain writable while the organization is disabled. Please include editable local many-to-many fields, or derive the read-only set from the bound form, and add an extended-model regression.
Confidence: medium. The default Organization form has no affected editable M2M field; this concerns supported swapped Organization models.
| Q(organization__in=user.organizations_managed) | ||
| ) | ||
| return queryset.select_related() | ||
| return queryset.filter(organization__is_active=True).select_related() |
There was a problem hiding this comment.
A normal GET, modify, PUT cycle cannot re-enable a disabled organization that has an owner. The detail response includes that owner, but this queryset removes its membership before OrganizationDetailSerializer.validate() runs. Even if it reached validate(), the current code marks every submitted owner as changed, including the existing owner. Please allow the existing owner value during re-enable validation, compare it with the persisted owner, and extend the PUT regression with an organization that already has an owner.
| // single hardcoded id. | ||
| $("select[data-field-name='organization']").each(function () { | ||
| var orgSelect = $(this); | ||
| orgSelect.closest("form").on("submit", function () { |
There was a problem hiding this comment.
This callback closes over only the selects present at document ready. Django later clones the empty inline row, but the form callback still reads the original empty-template select, not the newly added one. A dynamically added nullable organization widget can submit literal null. Please register one form submit handler that finds all current organization selects at submission time and cover a dynamically added nullable inline.
Confidence: medium. The built-in membership inline is non-nullable; this affects downstream nullable organization inlines using this reusable widget.
| follow=True, | ||
| ) | ||
| org_user.refresh_from_db() | ||
| self.assertFalse(org_user.is_admin) |
There was a problem hiding this comment.
Please assert response.status_code == 403 here. The unchanged database value alone can also result from form validation failure, so it does not prove has_change_permission() blocked the POST. CodeRabbit's outstanding suggestion on this test is correct.
| Disabling an Organization | ||
| ------------------------- | ||
|
|
||
| Superusers and managers of the organization can disable it, by unchecking |
There was a problem hiding this comment.
Org managers can disable the org? Can they also enable it again?
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
0894484 to
5b70bce
Compare
| if key != "owner" and getattr(self.instance, key) != data[key] | ||
| } | ||
| if owner_present: | ||
| changed_keys.add("owner") |
There was a problem hiding this comment.
WARNING: Unconditionally marking owner as changed blocks re-enabling with existing owner
changed_keys.add("owner") runs whenever owner is present in the payload, so the downstream condition (not owner_present or is_owner_unassignment) rejects a disabled-organization PUT that re-enables it while keeping the current owner. The test test_reenable_disabled_organization_with_existing_owner_via_put_api expects this to succeed (200), but the serializer blocks it.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Re-sending an unchanged membership of a disabled organization is | ||
| # the only REST path to remove it: the "is_admin" toggle-delete | ||
| # contract still applies once the field resolves. | ||
| data = {"organization_users": [{"is_admin": False, "organization": org1.pk}]} |
There was a problem hiding this comment.
WARNING: Test expects disabled organization pk to pass field validation
OrgUserCustomPrimarykeyRelatedField.get_queryset() uses Organization.active.all(), which excludes disabled organizations per the project's documentation. Sending a disabled org pk in organization_users should fail field validation with 400, not succeed with 200.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "description": org1.description, | ||
| "email": org1.email, | ||
| "url": org1.url, | ||
| "owner": {"organization_user": org1_user1.pk}, |
There was a problem hiding this comment.
WARNING: Test expects disabled organization user pk to pass field validation
CustomPrimaryKeyRelatedField.get_queryset() filters OrganizationUser by organization__is_active=True, excluding users of disabled organizations. Sending a disabled org owner in the owner field should fail field validation with 400, not succeed with 200.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
5b70bce to
f0c1898
Compare
Flake8 and Test FailuresHello @pandafy,
|
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.