[Fix] Cascade deletion blocked by UpgradeOperation #256#391
Open
UltraBot05 wants to merge 7 commits intoopenwisp:masterfrom
Open
[Fix] Cascade deletion blocked by UpgradeOperation #256#391UltraBot05 wants to merge 7 commits intoopenwisp:masterfrom
UltraBot05 wants to merge 7 commits intoopenwisp:masterfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdded Sequence Diagram(s)sequenceDiagram
participant AdminUser as Admin User
participant Request as HTTP Request
participant AdminView as AdminView (has_delete_permission)
participant DB as Database
AdminUser->>Request: submit delete action (single or bulk)
Request->>AdminView: resolve URL & call has_delete_permission(request, obj)
AdminView-->>Request: check
alt superuser
AdminView->>Request: return True
else obj is None (cascade confirm) or URL ends with "_delete" or action == "delete_selected"
AdminView->>Request: return True
else
AdminView->>Request: return False
end
alt permission granted
Request->>DB: perform delete cascade
DB-->>AdminView: deletion result
AdminView-->>AdminUser: show success/redirect
else permission denied
AdminView-->>AdminUser: show permission denied
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: Issues Found in Existing Comments | Recommendation: Review existing warnings before merge OverviewThis PR implements Existing Inline CommentsThe following issues have already been flagged by previous reviewers and should be addressed:
Implementation ReviewWhat works well:
Areas of concern (see existing comments):
Files Reviewed (2 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 926-952: Add an integration test that uses Django's test client
instead of mocking to exercise the real admin delete flow: in the
TestUpgradeOperationInlineDeletePermission test case create a superuser, create
an Organization and related BatchUpgradeOperation/UpgradeOperation instances,
log in with client.force_login or client.login, then perform a client.get to the
admin delete confirmation view for the Organization (URL name
admin:openwisp_users_organization_delete) and client.post the confirmation form
to trigger the cascade delete; finally assert that the related
UpgradeOperation/BatchUpgradeOperation records are removed and that the response
redirects as expected. Ensure this replaces or augments the subTest that
currently stubs resolver_match/POST.get so the regression that occurred during
real admin deletes is covered by a full client-based flow referencing
UpgradeOperationInline and BatchUpgradeOperation.
- Around line 932-950: The test hard-codes admin URL names (e.g.,
"admin:firmware_upgrader_batchupgradeoperation_change" and
"admin:openwisp_users_organization_changelist") which breaks when models are
swapped; update the test to build resolver_match.url_name dynamically using the
models' metadata: use ModelClass._meta.app_label and ModelClass._meta.model_name
to compose the admin names (prefix "admin:" + app_label + "_" + model_name +
"_change" or "_changelist" / "_delete" as needed) before calling
inline.has_delete_permission, ensuring the assertions use these derived values
rather than literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ebd3acd1-aabc-48c6-8b5b-dd71c054d1ec
📒 Files selected for processing (2)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | 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.1.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
🔇 Additional comments (2)
openwisp_firmware_upgrader/admin.py (1)
246-254: LGTM: the permission gate matches the intended admin flows.Making
objoptional keeps the inline hook compatible with Django’s call pattern, and the_delete/delete_selectedchecks allow cascade and bulk deletions while still blocking deletion from the normal change view.openwisp_firmware_upgrader/tests/test_admin.py (1)
4-25: LGTM.The added imports are all used by the new inline-permission test and keep the test setup focused.
UltraBot05
force-pushed
the
issues/256-delete-org-blocked
branch
from
04ba94f to
9b18c1d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/admin.py (1)
246-253:⚠️ Potential issue | 🔴 CriticalRestrict the bulk-delete shortcut to actual changelist requests.
Line 252 trusts any POST carrying
action=delete_selected, so a crafted POST to a*_changepage can re-enable inline row deletion and bypass the single-row safeguard. Require a*_changelistroute before honoring that action.🔒 Proposed fix
def has_delete_permission(self, request, obj=None): if request.resolver_match: url_name = request.resolver_match.url_name # Allow cascade deletions from parent delete views or bulk / mass operations if url_name and url_name.endswith("_delete"): return True - if request.POST.get("action") == "delete_selected": + if ( + url_name + and url_name.endswith("_changelist") + and request.POST.get("action") == "delete_selected" + ): return True return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/admin.py` around lines 246 - 253, The bulk-delete shortcut currently trusts any POST with action="delete_selected" in has_delete_permission; restrict it by also verifying the current route is a changelist route: when checking request.POST.get("action") == "delete_selected", require request.resolver_match.url_name to end with "_changelist" (similar to the existing "_delete" check) before returning True. Update has_delete_permission to only allow the delete_selected shortcut for actual changelist routes using the existing request.resolver_match.url_name logic.openwisp_firmware_upgrader/tests/test_admin.py (1)
930-950:⚠️ Potential issue | 🟡 MinorDerive these stubbed admin names from model metadata.
These hard-coded
openwisp_users/firmware_upgraderlabels make the test brittle in swapped-model setups. Build the syntheticurl_namevalues from_meta.app_label/_meta.model_nameinstead of literals.♻️ Example refactor
class TestUpgradeOperationInlineDeletePermission(TestCase): def test_upgrade_operation_inline_delete_permission(self): inline = UpgradeOperationInline(BatchUpgradeOperation, AdminSite()) + batch_url_base = ( + f"{BatchUpgradeOperation._meta.app_label}_" + f"{BatchUpgradeOperation._meta.model_name}" + ) with self.subTest("cascade delete via parent delete view"): request = MagicMock() - request.resolver_match.url_name = "admin:openwisp_users_organization_delete" + request.resolver_match.url_name = f"{batch_url_base}_delete" request.POST.get.return_value = None self.assertTrue(inline.has_delete_permission(request)) with self.subTest("bulk delete via delete_selected action"): request = MagicMock() - request.resolver_match.url_name = ( - "admin:openwisp_users_organization_changelist" - ) + request.resolver_match.url_name = f"{batch_url_base}_changelist" request.POST.get.side_effect = lambda key, default=None: ( "delete_selected" if key == "action" else default ) self.assertTrue(inline.has_delete_permission(request)) with self.subTest("normal change view — delete must be blocked"): request = MagicMock() - request.resolver_match.url_name = ( - "admin:firmware_upgrader_batchupgradeoperation_change" - ) + request.resolver_match.url_name = f"{batch_url_base}_change" request.POST.get.return_value = None self.assertFalse(inline.has_delete_permission(request))Based on learnings, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/tests/test_admin.py` around lines 930 - 950, Tests use hard-coded admin URL name strings which break when models are swapped; change the test to derive these names from model metadata instead of literals. In the test file where you construct request.resolver_match.url_name for checks of inline.has_delete_permission, compute the app_label and model_name from the relevant model classes (e.g., use Build._meta.app_label and Build._meta.model_name for the batchchange/changelist views, and Device._meta.app_label / Device._meta.model_name for the organization/delete examples) — update BaseTestAdmin to expose these derived values (e.g., app_label and config_app_label) and replace all literal "openwisp_users"/"firmware_upgrader" strings with f"admin:{app_label}_{model_name}_change" or f"admin:{app_label}_{model_name}_changelist" etc., so the url_name always reflects the models' _meta values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 246-253: The bulk-delete shortcut currently trusts any POST with
action="delete_selected" in has_delete_permission; restrict it by also verifying
the current route is a changelist route: when checking
request.POST.get("action") == "delete_selected", require
request.resolver_match.url_name to end with "_changelist" (similar to the
existing "_delete" check) before returning True. Update has_delete_permission to
only allow the delete_selected shortcut for actual changelist routes using the
existing request.resolver_match.url_name logic.
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 930-950: Tests use hard-coded admin URL name strings which break
when models are swapped; change the test to derive these names from model
metadata instead of literals. In the test file where you construct
request.resolver_match.url_name for checks of inline.has_delete_permission,
compute the app_label and model_name from the relevant model classes (e.g., use
Build._meta.app_label and Build._meta.model_name for the batchchange/changelist
views, and Device._meta.app_label / Device._meta.model_name for the
organization/delete examples) — update BaseTestAdmin to expose these derived
values (e.g., app_label and config_app_label) and replace all literal
"openwisp_users"/"firmware_upgrader" strings with
f"admin:{app_label}_{model_name}_change" or
f"admin:{app_label}_{model_name}_changelist" etc., so the url_name always
reflects the models' _meta values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 914c0e82-e21a-45fd-b71e-0575038a5c6d
📒 Files selected for processing (2)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
UltraBot05
force-pushed
the
issues/256-delete-org-blocked
branch
from
9b18c1d to
c41ab6a
Compare
Implemented has_delete_permission to allow deletions when triggered by a parent object or bulk actions. Mirrors logic from openwisp-utils #596. Added unit tests to verify the permission logic. Fixes openwisp#256
UltraBot05
force-pushed
the
issues/256-delete-org-blocked
branch
from
c41ab6a to
21bd3fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 326-346: The has_delete_permission implementation in
openwisp_firmware_upgrader.admin (method has_delete_permission) duplicates the
same logic in UpgradeOperationInline; extract the shared logic into a new mixin
(e.g., CascadeDeletePermissionMixin) that implements has_delete_permission, then
have both the current admin class and UpgradeOperationInline inherit from that
mixin; ensure the mixin preserves the existing checks (superuser short-circuit,
obj None handling, resolver_match URL suffix checks for "_delete" and
"_changelist" with delete_selected) and remove the duplicated method from both
classes so they rely on the mixin.
- Around line 246-266: In has_delete_permission, the check "elif obj is None" is
nested under the falsy request.resolver_match branch so it won't run during
admin cascade delete confirmations which do have a resolver_match; change the
logic so the obj is None check is evaluated independently (e.g., an explicit "if
obj is None: return True" placed after the superuser check and
before/independent of the resolver_match handling) while keeping the existing
resolver_match/url_name checks and superuser short-circuit intact.
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 927-958: The test misses coverage for the branches in
UpgradeOperationInline.has_delete_permission where request.resolver_match is
None (obj is None fallback) and where the request.user is a superuser
(early-return true); update
TestUpgradeOperationInlineDeletePermission.test_upgrade_operation_inline_delete_permission
to add two subTests: one that sets request.resolver_match = None and calls
inline.has_delete_permission(request, obj=None) asserting True, and another that
sets request.user.is_superuser = True with a resolver_match.url_name and calls
inline.has_delete_permission(request, obj=MagicMock()) asserting True so both
branches in has_delete_permission are exercised.
- Around line 960-974: The current test test_cascade_delete_integration only
covers superuser flow via _login(); add a second integration test that logs in
as a non-superuser (e.g., an organization admin), grants or assigns the
necessary admin delete permission for the Organization, and then performs the
same POST to the admin delete URL to verify the URL-based permission logic and
cascade deletion; locate the has_delete_permission method in admin.py and the
Organization model to ensure the test sets up the org-admin user and permissions
correctly, then assert the Organization and related BatchUpgradeOperation are
(or are not) deleted as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b355d147-72f5-40f4-b4a8-2ceb1aa837a9
📒 Files selected for processing (2)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
🔇 Additional comments (1)
openwisp_firmware_upgrader/tests/test_admin.py (1)
4-8: LGTM!Imports are appropriate for the new test cases.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/admin.py (2)
612-632: 🛠️ Refactor suggestion | 🟠 MajorDRY violation: Identical permission logic duplicated.
This method is an exact copy of
UpgradeOperationInline.has_delete_permission. Additionally, it has the same logic issue where theelif obj is Nonebranch (line 630) is unreachable during admin requests.Extract the shared logic into a reusable mixin to avoid maintenance burden and ensure consistency.
♻️ Proposed refactor using a mixin
class CascadeDeletePermissionMixin: """ Mixin that allows cascade/bulk deletions while blocking single-row deletion in the change view. """ def has_delete_permission(self, request, obj=None): if getattr(getattr(request, "user", None), "is_superuser", False) is True: return True if obj is None: return True if request.resolver_match: url_name = request.resolver_match.url_name if url_name and url_name.endswith("_delete"): return True if ( url_name and url_name.endswith("_changelist") and request.POST.get("action") == "delete_selected" ): return True return FalseThen inherit from it:
-class UpgradeOperationInline(admin.StackedInline): +class UpgradeOperationInline(CascadeDeletePermissionMixin, admin.StackedInline): model = UpgradeOperation - # ... remove the has_delete_permission method-class BatchUpgradeOperationAdmin(ReadonlyUpgradeOptionsMixin, ReadOnlyAdmin, BaseAdmin): +class BatchUpgradeOperationAdmin(CascadeDeletePermissionMixin, ReadonlyUpgradeOptionsMixin, ReadOnlyAdmin, BaseAdmin): # ... remove the has_delete_permission method🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/admin.py` around lines 612 - 632, The has_delete_permission implementation in openwisp_firmware_upgrader.admin (duplicated from UpgradeOperationInline.has_delete_permission) should be extracted into a shared CascadeDeletePermissionMixin to remove duplication; create a mixin (e.g., CascadeDeletePermissionMixin) that implements has_delete_permission with the correct branch order (check superuser, then if obj is None return True, then evaluate request.resolver_match/url_name cascade or bulk delete checks) and update both the current admin class and UpgradeOperationInline to inherit this mixin and remove their local has_delete_permission implementations.
342-345:⚠️ Potential issue | 🟠 MajorLogic issue:
elif obj is Nonebranch is unreachable during cascade delete.The
elif obj is Nonebranch (line 344) only executes whenrequest.resolver_matchis falsy. However, Django admin requests always have a validresolver_match, so this branch will never execute during cascade delete confirmation pages.The
obj is Nonecheck should be independent of theresolver_matchcondition to properly handle the cascade delete confirmation scenario.🔧 Proposed fix
def has_delete_permission(self, request, obj=None): # Superusers can delete anything (but skip if mocked) if getattr(getattr(request, "user", None), "is_superuser", False) is True: return True + # Allow if obj is None (general permission check, not instance-specific) + if obj is None: + return True # Check if this is a cascade delete from a parent view or bulk action if request.resolver_match: url_name = request.resolver_match.url_name # Allow cascade deletions from parent delete views or bulk / mass operations if url_name and url_name.endswith("_delete"): return True if ( url_name and url_name.endswith("_changelist") and request.POST.get("action") == "delete_selected" ): return True - # Allow if obj is None (cascade delete check during confirmation page) - # or if we can't determine the URL (internal permission check) - elif obj is None: - return True return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/admin.py` around lines 342 - 345, The current branch combines the resolver_match check with the obj is None check making "elif obj is None" unreachable; instead, handle the cascade-delete case independently by checking "if obj is None: return True" before or as a separate conditional from the resolver_match logic that uses request.resolver_match, ensuring the obj None case is evaluated regardless of request.resolver_match; update the block that currently reads "elif obj is None" (the conditional using request.resolver_match) to use a standalone "if obj is None: return True" so cascade delete confirmation pages are allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 1489-1516: Add two missing subtests to fully exercise
inline.has_delete_permission: one that sets request.user.is_superuser = True
(explicit boolean, not a MagicMock) and calls
inline.has_delete_permission(request, obj=MagicMock()) to hit the superuser
early-return path; and another that sets request.user.is_superuser = False and
request.resolver_match = None and calls inline.has_delete_permission(request,
obj=None) to exercise the fallback when resolver_match is falsy. Ensure the
tests reference the same inline object used elsewhere and keep the subTest
labels similar to existing ones for clarity.
- Around line 1491-1494: The tests set request.resolver_match.url_name
incorrectly including the "admin:" namespace; update assignments that build the
URL name (the ones using
f"admin:{Organization._meta.app_label}_{Organization._meta.model_name}_delete")
to remove the "admin:" prefix so they assign
f"{Organization._meta.app_label}_{Organization._meta.model_name}_delete"
instead; apply the same change for the two other occurrences that construct URL
names (the other similar f-strings near the blocks that reference
resolver_match.url_name).
---
Duplicate comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 612-632: The has_delete_permission implementation in
openwisp_firmware_upgrader.admin (duplicated from
UpgradeOperationInline.has_delete_permission) should be extracted into a shared
CascadeDeletePermissionMixin to remove duplication; create a mixin (e.g.,
CascadeDeletePermissionMixin) that implements has_delete_permission with the
correct branch order (check superuser, then if obj is None return True, then
evaluate request.resolver_match/url_name cascade or bulk delete checks) and
update both the current admin class and UpgradeOperationInline to inherit this
mixin and remove their local has_delete_permission implementations.
- Around line 342-345: The current branch combines the resolver_match check with
the obj is None check making "elif obj is None" unreachable; instead, handle the
cascade-delete case independently by checking "if obj is None: return True"
before or as a separate conditional from the resolver_match logic that uses
request.resolver_match, ensuring the obj None case is evaluated regardless of
request.resolver_match; update the block that currently reads "elif obj is None"
(the conditional using request.resolver_match) to use a standalone "if obj is
None: return True" so cascade delete confirmation pages are allowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bbbe8b50-f10b-430a-9574-43f1022f125d
📒 Files selected for processing (2)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/admin.py
🔇 Additional comments (1)
openwisp_firmware_upgrader/tests/test_admin.py (1)
1518-1532: Integration test validates cascade deletion for superusers only.The
_login()method logs in as a superuser, so this test exercises the superuser early-return path inhas_delete_permission. Since the original issue#256may have affected non-superuser organization admins as well, consider adding an integration test with a non-superuser to verify the URL-based permission logic works in a real request context.However, the test does effectively validate that the cascade delete flow works end-to-end, which provides confidence that issue
#256is fixed for superusers.
UltraBot05
force-pushed
the
issues/256-delete-org-blocked
branch
from
0b69191 to
e683bb0
Compare
UltraBot05
force-pushed
the
issues/256-delete-org-blocked
branch
from
e683bb0 to
e684f6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 70-80: The allowlist currently treats any url_name ending with
"_delete" as safe, which also permits this admin's own single-row delete; update
the guard that uses request.resolver_match.url_name so it excludes the admin's
own delete view by comparing against the admin model's expected admin url_name
(build using self.model._meta.app_label and self.model._meta.model_name), e.g.
only allow when url_name.endswith("_delete") AND url_name !=
f"{self.model._meta.app_label}_{self.model._meta.model_name}_delete"; keep the
existing bulk-changelist check as-is.
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 1562-1570: The test currently accepts both 200 and 403 outcomes
which masks regressions; modify the test around the self.client.post(delete_url,
...) call to set an explicit permission state for the test user (either make the
user a superuser or assign the specific permission required to delete the
Organization and BatchUpgradeOperation) and then assert only the expected result
(e.g. assertEqual(response.status_code, 200) and assert the
Organization/BatchUpgradeOperation no longer exist, or
assertEqual(response.status_code, 403) and assert they still exist). Update
references to delete_url, self.client.post,
Organization.objects.filter(pk=org.pk).exists(), and
BatchUpgradeOperation.objects.filter(pk=batch.pk).exists() accordingly and
remove the conditional branch that accepts both statuses.
- Around line 1489-1506: The two subtests call
inline.has_delete_permission(request) without an obj, so the function returns
early at the "obj is None" guard and never exercises the URL-based branches;
update both subtests to pass a non-None obj (e.g., a simple Organization
instance or a MagicMock standing in for the model instance) when calling
inline.has_delete_permission so the logic that inspects
request.resolver_match.url_name (using Organization._meta.app_label and
Organization._meta.model_name) and request.POST.get("action") for
"delete_selected" is actually executed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14c53784-0f5f-425d-86bc-5e2d2d895a00
📒 Files selected for processing (2)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
Author
|
@nemesifier Hi! Apologies for the silence from my end for the past few days - I was tied up with University Exams! I've pushed the latest updates to handle the This pairs with #596 over at openwisp-utils to fully resolve the issue. The CI is completely green, so it's ready for your review whenever you have time! |
4 tasks
UltraBot05
changed the title
Viscous106
reviewed
Mar 13, 2026
There was a problem hiding this comment.
@UltraBot05 LGTM ! All the test case are running and the bug seem to have fixed on testing on my local machine there is just one minute change i have suggested above .
UltraBot05
force-pushed
the
issues/256-delete-org-blocked
branch
from
2163ddc to
69a9d49
Compare
{
"failures": [
{
"type": "Build/Infrastructure/Other",
"message": "The installation of `openwisp-ipam` failed due to repeated 502 errors from GitHub.",
"details": "HTTPSConnectionPool(host='github.com', port=443): Max retries exceeded with url: /openwisp/openwisp-ipam/archive/refs/heads/1.3.tar.gz (Caused by ResponseError('too many 502 error responses'))"
}
]
}Build/Infrastructure/OtherHello @UltraBot05, The CI build failed because it was unable to download the Fix: |
UltraBot05
force-pushed
the
issues/256-delete-org-blocked
branch
from
560ff13 to
c9d2867
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implemented has_delete_permission to allow deletions when triggered by a parent object or bulk actions. This mirrors the logic from openwisp-utils #596.
Added unit tests to verify the permission logic.
Fixes #256
Checklist
Reference to Existing Issue
Closes #256.
Description of Changes
Fixes the Organization deletion blocker caused by UpgradeOperation and BatchUpgradeOperation records.
Key changes:
Extracted permission rules into a shared
CascadeDeletePermissionMixinapplied to bothUpgradeOperationInlineandBatchUpgradeOperationAdmin.The mixin uses
request.resolver_match.url_nameto allow parent/bulk deletes, but strictly blocks the model's own single-row delete and change views to prevent spoofing.Fixed the
obj is Nonefallback vulnerability flagged during review.Added a non-superuser integration test (
test_cascade_delete_integration_non_superuser) to verify the URL routing logic end-to-end without relying on superuser bypasses.This PR is the companion to openwisp-utils#596.
Screenshot
Before these changes:
After These changes:
