[change] Limit controller operations on disabled organizations #1393 - #1456
[change] Limit controller operations on disabled organizations #1393#1456pandafy wants to merge 8 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:
📝 WalkthroughWalkthroughThe change restricts controller operations for inactive organizations. It blocks registration, imports, writes, template selection, API mutations, provisioning, WHOIS, location updates, VPN webhooks, and PKI actions. Organization deactivation now asynchronously deactivates active devices and invalidates caches after transaction commit. Tests cover the updated admin, API, controller, task, handler, PKI, and provisioning behavior. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ControllerAPI
participant Organization
participant Device
Client->>ControllerAPI: submit controller operation
ControllerAPI->>Organization: check active status
Organization-->>ControllerAPI: active or inactive
alt organization inactive
ControllerAPI-->>Client: reject or skip operation
else organization active
ControllerAPI->>Device: apply operation
Device-->>ControllerAPI: operation result
ControllerAPI-->>Client: return response
end
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
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). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_controller/geo/api/views.py (1)
107-130: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd
organizationto the queryset'sselect_related.
device.organization.is_activeat Line 128 triggers an extra query per non-GET/HEADrequest becauseorganizationis not in the queryset'sselect_related. Add it, consistent with theselect_related("organization")already added toDeviceLocationView.get_parent_querysetin this same diff.⚡ Proposed fix
queryset = Device.objects.select_related( - "devicelocation", "devicelocation__location" + "devicelocation", "devicelocation__location", "organization" )🤖 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_controller/geo/api/views.py` around lines 107 - 130, Update DeviceCoordinatesView.queryset to include organization in the existing select_related chain so get_object can reuse the prefetched device.organization when checking device.organization.is_active. Keep the current devicelocation and devicelocation__location eager loading intact, and make the change on the DeviceCoordinatesView queryset rather than in get_object or the permission check path.openwisp_controller/config/admin.py (1)
1207-1231: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPin a dependency revision that provides the disabled-organization APIs and test mixins.
requirements.txtpinsopenwisp-usersto1.3, which provides neitherOrganization.activenorTestDisabledOrgApiMixinor the requested admin helpers. PR#542remains open and does not add these declarations. Pin a revision that provides all required symbols, or implement them locally.🤖 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_controller/config/admin.py` around lines 1207 - 1231, Update the dependency declaration for openwisp-users so it pins a revision exposing Organization.active, TestDisabledOrgApiMixin, and the required admin helpers, then verify the usages in openwisp_controller/config/admin.py (save_clones), openwisp_controller/config/tests/test_api.py, and openwisp_controller/config/tests/test_admin.py resolve against that revision; alternatively implement all missing symbols locally.
🤖 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_controller/config/handlers.py`:
- Around line 208-210: Update the _on_commit workflow in handlers.py so cache
invalidation cannot run before device deactivation completes. Use the existing
task symbols deactivate_organization_devices and
invalidate_controller_views_cache to create one ordered Celery workflow, such as
a chain or a follow-up step inside deactivate_organization_devices, and keep the
cache invalidation as the final action after deactivation finishes.
In `@openwisp_controller/config/tests/test_handlers.py`:
- Around line 45-64: Extend test_deactivate_organization_devices with two
devices in the same organization and make one device.deactivate() raise an
exception. Assert the other device is still deactivated or processed, and verify
the failure is logged while the task continues without propagating the
per-device exception.
In `@openwisp_controller/pki/admin.py`:
- Around line 58-61: Update revoke_action in openwisp_controller/pki/admin.py to
pass the original queryset into super().revoke_action() instead of filtering it
through _exclude_disabled_org, so revocation still works for disabled
organizations; keep the disabled-organization filtering behavior only where it
is needed for renewal. In openwisp_controller/pki/tests/test_admin.py, adjust
the admin test to assert that revocation succeeds for certificates in disabled
organizations while preserving the existing renewal assertion.
---
Outside diff comments:
In `@openwisp_controller/config/admin.py`:
- Around line 1207-1231: Update the dependency declaration for openwisp-users so
it pins a revision exposing Organization.active, TestDisabledOrgApiMixin, and
the required admin helpers, then verify the usages in
openwisp_controller/config/admin.py (save_clones),
openwisp_controller/config/tests/test_api.py, and
openwisp_controller/config/tests/test_admin.py resolve against that revision;
alternatively implement all missing symbols locally.
In `@openwisp_controller/geo/api/views.py`:
- Around line 107-130: Update DeviceCoordinatesView.queryset to include
organization in the existing select_related chain so get_object can reuse the
prefetched device.organization when checking device.organization.is_active. Keep
the current devicelocation and devicelocation__location eager loading intact,
and make the change on the DeviceCoordinatesView queryset rather than in
get_object or the permission check path.
🪄 Autofix
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: 8d4c6809-f2cc-4ae8-9ecc-9204325dc5ae
📒 Files selected for processing (24)
.github/workflows/ci.ymlopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.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.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside itIn Django pull requests, mark all user-facing strings as translatable using the Django internationalization framework.
Files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/exportable.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
**/*
⚙️ CodeRabbit configuration file
**/*: - Flag potential security vulnerabilities
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Flag unused or redundant code
Flag outdated or incorrect comments/docstrings
Ensure new code handles errors properly:
- Log errors that cannot be resolved by the user with error level
- Log unusual conditions with warning level
- Log important background actions with info level
- Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/exportable.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
**/*tests*/**
⚙️ CodeRabbit configuration file
**/*tests*/**: Ensure tests cover relevant success, error, boundary, and unusual
input scenarios.Flag tests that depend on arbitrary sleeps, uncontrolled system time,
specific timezones, unseeded randomness, network access, external
services, execution order, shared mutable state, hardcoded ports, or
asynchronous operations that are not properly awaited.
Files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
.github/**
⚙️ CodeRabbit configuration file
.github/**: Do not complain about dependencies installed from controlled mutable
OpenWISP branches. Branch protection restricts changes to those
branches.
Files:
.github/workflows/ci.yml
🧠 Learnings (11)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/exportable.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/exportable.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/exportable.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 ast-grep (0.45.0)
openwisp_controller/config/tasks.py
[warning] 235-235: 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("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 OpenGrep (1.26.0)
openwisp_controller/config/admin.py
[WARNING] 888-894: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (23)
.github/workflows/ci.yml (1)
75-75: 🗄️ Data Integrity & IntegrationInstall the dependency revision that contains PR
#542.Line 75 installs the
openwisp-usersdefault branch. If PR#542is not merged into that branch, CI does not test the dependency required by this PR. It can fail for the wrong reason or miss an API contract failure.Use the PR ref or an exact commit SHA.
Example
- pip install --upgrade --force-reinstall --no-deps --no-cache-dir "https://github.com/openwisp/openwisp-users/tarball/" + pip install --upgrade --force-reinstall --no-deps --no-cache-dir "https://github.com/openwisp/openwisp-users/archive/refs/pull/542/head.tar.gz"Based on the PR objective and the supplied
requirements.txt:8contract, CI must test the dependency from PR#542. As per path instructions, mutable refs in controlled OpenWISP repositories are allowed; this finding concerns the missing PR-specific ref, not mutability.Source: Path instructions
openwisp_controller/config/tasks.py (1)
129-138: LGTM!Also applies to: 229-249
openwisp_controller/config/tests/test_handlers.py (1)
3-44: LGTM!openwisp_controller/connection/api/serializers.py (1)
24-24: LGTM!openwisp_controller/connection/api/views.py (1)
41-44: LGTM!Also applies to: 129-132
openwisp_controller/mixins.py (1)
3-7: LGTM!Also applies to: 31-35
openwisp_controller/config/whois/tasks.py (1)
75-80: LGTM!Also applies to: 91-100
openwisp_controller/geo/estimated_location/tasks.py (1)
32-36: LGTM!openwisp_controller/subnet_division/tasks.py (1)
137-150: LGTM!openwisp_controller/pki/admin.py (1)
1-4: LGTM!Also applies to: 17-40, 53-56
openwisp_controller/pki/tests/test_admin.py (1)
148-165: LGTM!openwisp_controller/config/tests/test_vpn.py (1)
820-832: LGTM!openwisp_controller/config/admin.py (1)
604-607: LGTM!Also applies to: 725-748, 873-901
openwisp_controller/config/api/serializers.py (1)
111-126: LGTM!openwisp_controller/config/controller/views.py (1)
416-421: LGTM!openwisp_controller/config/exportable.py (1)
118-133: 📐 Maintainability & Code Quality | ⚡ Quick winWrap the new validation message for translation.
The message
"Cannot import rows for disabled organizations."is a raw string. Wrap it with Django's translation helper, as done for the other user-facing strings added in this PR (e.g.admin.py's "Selected organization is disabled.").Based on learnings, this repo requires Django i18n helpers for user-facing strings in Django code.🌐 Proposed fix
-from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.utils.translation import gettext_lazy as _raise ValidationError( - {"organization_id": "Cannot import rows for disabled organizations."} + { + "organization_id": _( + "Cannot import rows for disabled organizations." + ) + } )Source: Coding guidelines
openwisp_controller/config/tests/test_api.py (1)
243-259: LGTM!Also applies to: 614-715
openwisp_controller/config/tests/test_controller.py (2)
1439-1457: 📐 Maintainability & Code Quality | ⚡ Quick winFix the confusing/typo comment before the failing assertion.
The comment "Device can fetch checksum untill the device is deactivated" precedes an assertion expecting
404. As written, the comment reads as though the fetch should still succeed at this point, and it contains a typo ("untill"). Clarify that disabling the organization triggers the device's asynchronous deactivation, which is why the checksum fetch now fails.As per path instructions, "Flag outdated or incorrect comments/docstrings."📝 Proposed fix
- # Device can fetch checksum untill the device is deactivated + # Disabling the organization queues asynchronous device deactivation; + # once deactivated, the checksum endpoint returns 404. response = self.client.get(Source: Path instructions
1224-1240: LGTM!Also applies to: 1458-1469
openwisp_controller/geo/api/views.py (1)
155-180: LGTM!openwisp_controller/config/whois/tests/tests.py (1)
1061-1072: LGTM!openwisp_controller/geo/estimated_location/tests/tests.py (1)
1029-1041: LGTM!openwisp_controller/subnet_division/tests/test_models.py (1)
683-698: LGTM!
| @action(description=_("Revoke selected certificates"), permissions=["change"]) | ||
| def revoke_action(self, request, queryset): | ||
| queryset = _exclude_disabled_org(self, request, queryset) | ||
| return super().revoke_action(request, queryset) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Locate and read the revoke_action implementation in pki/admin.py
fd "admin.py" openwisp_controller/pki --type fRepository: openwisp/openwisp-controller
Length of output: 243
🏁 Script executed:
# Read the PKI admin implementation around revoke_action
cat -n openwisp_controller/pki/admin.py | sed -n '1,80p'Repository: openwisp/openwisp-controller
Length of output: 3048
🏁 Script executed:
# Find the _exclude_disabled_org function definition
rg "_exclude_disabled_org" -B 3 -A 10 openwisp_controller/Repository: openwisp/openwisp-controller
Length of output: 3309
🏁 Script executed:
# Check the parent class and inherited revoke_action
rg "def revoke_action" -B 3 -A 15 openwisp_controller/Repository: openwisp/openwisp-controller
Length of output: 1143
🏁 Script executed:
# Read the test expectations
cat -n openwisp_controller/pki/tests/test_admin.py | sed -n '160,200p'Repository: openwisp/openwisp-controller
Length of output: 1898
🏁 Script executed:
# Search for django_x509 AbstractCertAdmin or other base class behavior
rg "AbstractCertAdmin|revoke_action" openwisp_controller/ -lRepository: openwisp/openwisp-controller
Length of output: 243
🏁 Script executed:
# Check if there are any other revoke-related filters in the codebase
rg "revoke" openwisp_controller/pki/admin.py -i -B 2 -A 2Repository: openwisp/openwisp-controller
Length of output: 475
🏁 Script executed:
# Verify the parent class chain and MRO implications
cat -n openwisp_controller/pki/admin.py | sed -n '48,56p'Repository: openwisp/openwisp-controller
Length of output: 646
🏁 Script executed:
# Check the issue reference to understand the business requirement
rg "1393|issue.*1393" openwisp_controller/ -i -B 2 -A 2 2>/dev/null || echo "Issue reference not found in code"Repository: openwisp/openwisp-controller
Length of output: 200
🏁 Script executed:
# Check if there are any other safeguards on revoke in parent or sibling code
rg "revoke" openwisp_controller/ --type py -l | head -10Repository: openwisp/openwisp-controller
Length of output: 578
🏁 Script executed:
# Check if there's documentation or comments about why disabled orgs are restricted
rg "disabled.*organization|organization.*disabled" openwisp_controller/ -B 2 -A 2 -i | head -50Repository: openwisp/openwisp-controller
Length of output: 3475
🏁 Script executed:
# Look for any comments or history about the disabled org restriction
rg "is_active" openwisp_controller/pki/ --type py -B 2 -A 2Repository: openwisp/openwisp-controller
Length of output: 2269
🏁 Script executed:
# Check if the parent AbstractCertAdmin is from django_x509 external package
rg "from django_x509" openwisp_controller/pki/admin.pyRepository: openwisp/openwisp-controller
Length of output: 236
🏁 Script executed:
# Understand the intent: is revocation a special operation? Check API or other uses
rg "revoke" openwisp_controller/pki/ --type py -B 3 -A 3Repository: openwisp/openwisp-controller
Length of output: 7976
🏁 Script executed:
# Check if MultitenantAdminMixin applies any filters to revoke_action
rg "revoke_action|class MultitenantAdminMixin" openwisp_controller/admin.py -B 2 -A 10Repository: openwisp/openwisp-controller
Length of output: 332
🏁 Script executed:
# Search for documentation or comments about disabled organizations in the broader context
rg "disabled.*org|organization.*disabled" openwisp_controller/ --type py | rg -i "revoke|security|emergency|cleanup" -B 2 -A 2Repository: openwisp/openwisp-controller
Length of output: 166
🏁 Script executed:
# Check the API revoke endpoint to see if it applies disabled org filtering
cat -n openwisp_controller/pki/api/views.py | sed -n '1,150p' | tail -100Repository: openwisp/openwisp-controller
Length of output: 3788
🏁 Script executed:
# Look for any permission or authorization checks on cert_revoke API
rg "cert_revoke|CertRevokeView" openwisp_controller/pki/api/views.py -B 5 -A 20Repository: openwisp/openwisp-controller
Length of output: 1217
🏁 Script executed:
# Check if there's a model-level revoke method that might override behavior
rg "def revoke\(" openwisp_controller/pki/ --type py -B 3 -A 10Repository: openwisp/openwisp-controller
Length of output: 166
🏁 Script executed:
# Search for any comments explaining the rationale for disabled org filtering
rg "exclude.*disabled|skip.*disabled" openwisp_controller/ --type py -B 5 -A 5Repository: openwisp/openwisp-controller
Length of output: 11312
Security Misconfiguration (CWE-693)
Reachability: External · Exploitability: Moderate
Allow certificate revocation for disabled organizations.
_exclude_disabled_org removes selected certificates before super().revoke_action() runs. Call the inherited action with the original queryset. Keep disabled-organization filtering for renewal. Update the test to assert successful revocation and retain the renewal assertion.
📍 Affects 2 files
openwisp_controller/pki/admin.py#L58-L61(this comment)openwisp_controller/pki/tests/test_admin.py#L167-L192
🤖 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_controller/pki/admin.py` around lines 58 - 61, Update revoke_action
in openwisp_controller/pki/admin.py to pass the original queryset into
super().revoke_action() instead of filtering it through _exclude_disabled_org,
so revocation still works for disabled organizations; keep the
disabled-organization filtering behavior only where it is needed for renewal. In
openwisp_controller/pki/tests/test_admin.py, adjust the admin test to assert
that revocation succeeds for certificates in disabled organizations while
preserving the existing renewal assertion.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
|
03745c3 to
f33e3d1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 (3)
openwisp_controller/config/controller/views.py (2)
416-421: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd
organizationtoselect_relatedto avoid an extra query.
device.organization.is_activeis accessed right after fetchingdevice, but the query only usesselect_related("config"). This causes an extra database query on every request where an existing device is found by key.See the consolidated comment for the fix (shared with
openwisp_controller/geo/api/views.py).🤖 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_controller/config/controller/views.py` around lines 416 - 421, Update the device lookup in the controller view to include the organization relation alongside config in select_related, so the subsequent device.organization.is_active check uses the same query. Preserve the existing filtering and response behavior.
1-1: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMissing
select_related("organization")causes a new N+1 query at two sites. Both sites now accessdevice.organization.is_activeto guard mutating operations, but neither extends its existingselect_relatedto includeorganization, so each request now issues one extra query.
openwisp_controller/config/controller/views.py#L416-421: changeself.model.objects.select_related("config").get(key=key)toself.model.objects.select_related("config", "organization").get(key=key).openwisp_controller/geo/api/views.py#L110-130: change theDeviceCoordinatesView.querysetclass attribute fromDevice.objects.select_related("devicelocation", "devicelocation__location")to also include"organization".🤖 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_controller/config/controller/views.py` at line 1, Extend the existing select_related calls at both affected sites to preload organization: in the view method retrieving by key, include "organization" alongside "config", and in DeviceCoordinatesView.queryset include "organization" alongside the existing location relations. Preserve all other queryset behavior.openwisp_controller/config/admin.py (1)
1207-1224: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace
Organization.activewith a supported active-organization query. The1.3dependency branch does not define this manager, so template cloning raisesAttributeError. UseOrganization.objectswithis_active=True, or update and immutably pinopenwisp-usersto a version that definesactive.🤖 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_controller/config/admin.py` around lines 1207 - 1224, Update the organization lookup in save_clones to use the supported Organization.objects query filtered by is_active=True instead of Organization.active. Preserve the existing handling for ValidationError and Organization.DoesNotExist, including the warning and user-facing error response.
🤖 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_controller/config/exportable.py`:
- Around line 131-133: Wrap the user-facing validation message in the
ValidationError raised by the export/import validation flow with Django’s
standard lazy translation helper, consistent with existing imports and
translated admin messages.
- Around line 124-133: Cache the active-state lookup in the import validation
flow surrounding the organization_id check, keyed by organization ID and scoped
to the duration of the import. Reuse cached results for repeated organization
IDs, querying Organization.objects only on the first encounter while preserving
the existing ValidationError for inactive organizations.
In `@openwisp_controller/config/tests/test_controller.py`:
- Around line 1224-1240: Update test_register_reregistration_403_disabled_org to
create or reuse a separate active organization and submit that organization’s
shared secret in the reregistration payload, while keeping the device assigned
to the disabled organization. Assert the request still returns the forbidden
response, ensuring execution reaches the device.organization.is_active guard
rather than rejecting the secret in DeviceRegisterView.forbidden().
- Around line 1439-1456: Update the comment in test_checksum_404_disabled_org to
state that the device cannot fetch the checksum once its organization is
deactivated, correcting “untill” and aligning the wording with the expected 404
response.
In `@openwisp_controller/config/whois/tasks.py`:
- Line 77: Update the WHOIS querysets in
openwisp_controller/config/whois/tasks.py at lines 77-77 and 96-96 to eager-load
organization with select_related("organization"); on the locked WHOIS queryset,
also restrict select_for_update to of=("self",). Update the estimated-location
queryset in openwisp_controller/geo/estimated_location/tasks.py at lines 32-36
to select_related("organization").
In `@openwisp_controller/geo/api/views.py`:
- Around line 125-130: Update the queryset definition for the view containing
get_object to include organization in select_related, ensuring
device.organization.is_active uses the already-loaded relation without an extra
query while preserving the existing get_object permission checks.
---
Outside diff comments:
In `@openwisp_controller/config/admin.py`:
- Around line 1207-1224: Update the organization lookup in save_clones to use
the supported Organization.objects query filtered by is_active=True instead of
Organization.active. Preserve the existing handling for ValidationError and
Organization.DoesNotExist, including the warning and user-facing error response.
In `@openwisp_controller/config/controller/views.py`:
- Around line 416-421: Update the device lookup in the controller view to
include the organization relation alongside config in select_related, so the
subsequent device.organization.is_active check uses the same query. Preserve the
existing filtering and response behavior.
- Line 1: Extend the existing select_related calls at both affected sites to
preload organization: in the view method retrieving by key, include
"organization" alongside "config", and in DeviceCoordinatesView.queryset include
"organization" alongside the existing location relations. Preserve all other
queryset behavior.
🪄 Autofix
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: 158fee74-15f4-4110-adfd-dc71b91296e9
📒 Files selected for processing (24)
.github/workflows/ci.ymlopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- 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.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside itIn Django pull requests, mark all user-facing strings as translatable using the Django internationalization framework.
Files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/mixins.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/handlers.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_admin.py
**/*
⚙️ CodeRabbit configuration file
**/*: - Flag potential security vulnerabilities
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Flag unused or redundant code
Flag outdated or incorrect comments/docstrings
Ensure new code handles errors properly:
- Log errors that cannot be resolved by the user with error level
- Log unusual conditions with warning level
- Log important background actions with info level
- Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/mixins.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/handlers.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_admin.py
**/*tests*/**
⚙️ CodeRabbit configuration file
**/*tests*/**: Ensure tests cover relevant success, error, boundary, and unusual
input scenarios.Flag tests that depend on arbitrary sleeps, uncontrolled system time,
specific timezones, unseeded randomness, network access, external
services, execution order, shared mutable state, hardcoded ports, or
asynchronous operations that are not properly awaited.
Files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.py
.github/**
⚙️ CodeRabbit configuration file
.github/**: Do not complain about dependencies installed from controlled mutable
OpenWISP branches. Branch protection restricts changes to those
branches.
Files:
.github/workflows/ci.yml
🧠 Learnings (11)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/mixins.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/handlers.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_admin.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/mixins.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/handlers.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_admin.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/mixins.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/handlers.pyopenwisp_controller/pki/admin.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_admin.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.py
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 ast-grep (0.45.0)
openwisp_controller/config/tasks.py
[warning] 235-235: 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("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 OpenGrep (1.26.0)
openwisp_controller/config/admin.py
[WARNING] 888-894: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (21)
openwisp_controller/config/handlers.py (1)
208-210: Keep the task execution order.
delay()does not order task execution across Celery workers. Cache invalidation can run before device deactivation completes. Use a Celery chain with immutable signatures, or dispatch invalidation afterdeactivate_organization_devicescompletes.openwisp_controller/config/tests/test_handlers.py (2)
45-64: Cover per-device deactivation failures.Add two devices for the disabled organization. Make one
device.deactivate()call fail. Assert that the other device is processed and that the exception is logged.As per path instructions, “Ensure tests cover relevant success, error, boundary, and unusual input scenarios.”
Source: Path instructions
3-43: LGTM!openwisp_controller/config/tasks.py (1)
129-138: LGTM!Also applies to: 229-249
openwisp_controller/connection/api/serializers.py (1)
24-24: LGTM!openwisp_controller/connection/api/views.py (1)
41-44: LGTM!Also applies to: 129-132
openwisp_controller/mixins.py (1)
3-7: LGTM!Also applies to: 31-35
.github/workflows/ci.yml (1)
75-75: LGTM!openwisp_controller/subnet_division/tasks.py (1)
137-150: LGTM!openwisp_controller/pki/admin.py (1)
1-56: LGTM!openwisp_controller/pki/tests/test_admin.py (1)
148-165: LGTM!openwisp_controller/config/tests/test_vpn.py (1)
820-832: LGTM!openwisp_controller/config/admin.py (1)
603-607: LGTM!Also applies to: 725-748, 873-901
openwisp_controller/config/api/serializers.py (1)
111-126: LGTM!openwisp_controller/geo/api/views.py (1)
178-180: LGTM!openwisp_controller/config/tests/test_admin.py (1)
122-140: LGTM!Also applies to: 688-726, 1006-1101
openwisp_controller/config/tests/test_api.py (1)
243-259: LGTM!Also applies to: 614-715
openwisp_controller/config/tests/test_controller.py (1)
66-91: LGTM!Also applies to: 272-272, 336-336, 1458-1470
openwisp_controller/config/whois/tests/tests.py (1)
1061-1072: LGTM!openwisp_controller/geo/estimated_location/tests/tests.py (1)
1029-1041: LGTM!openwisp_controller/subnet_division/tests/test_models.py (1)
682-698: LGTM!
| if ( | ||
| instance.organization_id | ||
| and not Organization.objects.filter( | ||
| pk=instance.organization_id, | ||
| is_active=True, | ||
| ).exists() | ||
| ): | ||
| raise ValidationError( | ||
| {"organization_id": "Cannot import rows for disabled organizations."} | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Cache organization-active state to avoid one query per imported row.
Organization.objects.filter(...).exists() runs once per row. For a large import where many rows share the same organization, this issues repeated, identical queries. Cache the result per organization id for the duration of the import.
♻️ Proposed caching fix
+ _active_org_cache = {}
+
def validate_instance(
self, instance, import_validation_errors=None, validate_unique=True
):
super().validate_instance(
instance, import_validation_errors=None, validate_unique=True
)
- if (
- instance.organization_id
- and not Organization.objects.filter(
- pk=instance.organization_id,
- is_active=True,
- ).exists()
- ):
+ org_id = instance.organization_id
+ if org_id and not self._active_org_cache.setdefault(
+ org_id,
+ Organization.objects.filter(pk=org_id, is_active=True).exists(),
+ ):
raise ValidationError(
{"organization_id": "Cannot import rows for disabled organizations."}
)🤖 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_controller/config/exportable.py` around lines 124 - 133, Cache the
active-state lookup in the import validation flow surrounding the
organization_id check, keyed by organization ID and scoped to the duration of
the import. Reuse cached results for repeated organization IDs, querying
Organization.objects only on the first encounter while preserving the existing
ValidationError for inactive organizations.
| def test_checksum_404_disabled_org(self): | ||
| org = self._create_org() | ||
| config = self._create_config(organization=org) | ||
| device = config.device | ||
| # Cache checksum | ||
| response = self.client.get( | ||
| reverse("controller:device_checksum", args=[device.pk]), | ||
| {"key": device.key}, | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| org.is_active = False | ||
| org.save() | ||
| # Device can fetch checksum untill the device is deactivated | ||
| response = self.client.get( | ||
| reverse("controller:device_checksum", args=[device.pk]), | ||
| {"key": device.key}, | ||
| ) | ||
| self.assertEqual(response.status_code, 404) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale comment and typo.
The comment "Device can fetch checksum untill the device is deactivated" contradicts the assertion below it, which expects 404 immediately after the organization is disabled, not after device deactivation. The comment also has a typo ("untill").
📝 Proposed fix
- # Device can fetch checksum untill the device is deactivated
+ # Checksum requests are rejected as soon as the organization is disabled
response = self.client.get(
reverse("controller:device_checksum", args=[device.pk]),
{"key": device.key},
)
self.assertEqual(response.status_code, 404)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_checksum_404_disabled_org(self): | |
| org = self._create_org() | |
| config = self._create_config(organization=org) | |
| device = config.device | |
| # Cache checksum | |
| response = self.client.get( | |
| reverse("controller:device_checksum", args=[device.pk]), | |
| {"key": device.key}, | |
| ) | |
| self.assertEqual(response.status_code, 200) | |
| org.is_active = False | |
| org.save() | |
| # Device can fetch checksum untill the device is deactivated | |
| response = self.client.get( | |
| reverse("controller:device_checksum", args=[device.pk]), | |
| {"key": device.key}, | |
| ) | |
| self.assertEqual(response.status_code, 404) | |
| def test_checksum_404_disabled_org(self): | |
| org = self._create_org() | |
| config = self._create_config(organization=org) | |
| device = config.device | |
| # Cache checksum | |
| response = self.client.get( | |
| reverse("controller:device_checksum", args=[device.pk]), | |
| {"key": device.key}, | |
| ) | |
| self.assertEqual(response.status_code, 200) | |
| org.is_active = False | |
| org.save() | |
| # Checksum requests are rejected as soon as the organization is disabled | |
| response = self.client.get( | |
| reverse("controller:device_checksum", args=[device.pk]), | |
| {"key": device.key}, | |
| ) | |
| self.assertEqual(response.status_code, 404) |
🤖 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_controller/config/tests/test_controller.py` around lines 1439 -
1456, Update the comment in test_checksum_404_disabled_org to state that the
device cannot fetch the checksum once its organization is deactivated,
correcting “untill” and aligning the wording with the expected 404 response.
Black and Flake8 FailuresHello @pandafy, The CI failed due to code style issues detected by Black and Flake8.
Please fix these issues and push a new commit. |
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_controller/geo/tests/test_api.py`:
- Around line 767-771: Strengthen the “location_device_list stays readable”
subtest by parsing the successful response and asserting it contains the
disabled-organization device’s identifier or name created by the test. Keep the
existing 200 status assertion, and target the response assertion around the
location_device_list request.
- Around line 978-1000: Update the DeviceLocation PUT-as-create permission flow
to enforce the organization write check before perform_create, including
requests routed through clone_request(..., "POST") and has_permission. Then
change test_create_devicelocation_disabled_org to expect HTTP 403 instead of
201.
In `@openwisp_controller/pki/tests/test_api.py`:
- Around line 250-263: Update test_cert_revoke_renew_api_disabled_org to capture
the certificate serial before both requests, refresh the certificate from the
database after each response, and assert revoked remains false and the serial is
unchanged after each action, while retaining the existing 403 assertions.
🪄 Autofix
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: 89c03822-a366-4c01-b366-8b6b4031ac44
📒 Files selected for processing (8)
openwisp_controller/config/admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/subnet_division/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside itIn Django pull requests, mark all user-facing strings as translatable using the Django internationalization framework.
Files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/admin.pyopenwisp_controller/connection/tests/test_api.py
**/*tests*/**
⚙️ CodeRabbit configuration file
**/*tests*/**: Ensure tests cover relevant success, error, boundary, and unusual
input scenarios.Flag tests that depend on arbitrary sleeps, uncontrolled system time,
specific timezones, unseeded randomness, network access, external
services, execution order, shared mutable state, hardcoded ports, or
asynchronous operations that are not properly awaited.
Files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
**/*
⚙️ CodeRabbit configuration file
**/*: - Flag potential security vulnerabilities
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Flag unused or redundant code
Flag outdated or incorrect comments/docstrings
Ensure new code handles errors properly:
- Log errors that cannot be resolved by the user with error level
- Log unusual conditions with warning level
- Log important background actions with info level
- Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/admin.pyopenwisp_controller/connection/tests/test_api.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/admin.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/admin.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/config/admin.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/geo/tests/test_admin.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
🪛 ast-grep (0.45.0)
openwisp_controller/connection/tests/test_api.py
[info] 194-194: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (11)
openwisp_controller/geo/tests/test_admin.py (4)
3-3: LGTM!Also applies to: 16-16
134-178: LGTM!
288-322: LGTM!
360-401: LGTM!openwisp_controller/subnet_division/tests/test_models.py (1)
683-699: LGTM!openwisp_controller/config/admin.py (1)
604-604: LGTM!Also applies to: 729-748, 879-901, 1009-1029, 1214-1226
openwisp_controller/connection/tests/test_admin.py (1)
4-4: LGTM!Also applies to: 15-15, 102-120, 260-277
openwisp_controller/connection/tests/test_api.py (1)
15-15: LGTM!Also applies to: 29-31, 188-216, 459-487, 622-655
openwisp_controller/geo/tests/test_api.py (1)
23-23: LGTM!Also applies to: 203-222, 418-419, 548-583, 712-750
openwisp_controller/pki/tests/test_admin.py (1)
148-231: LGTM!openwisp_controller/pki/tests/test_api.py (1)
8-8: LGTM!Also applies to: 22-22, 164-205, 264-280
|
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/admin.py (1)
603-607: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorization Bypass (CWE-862): Missing Authorization
Reachability: External · Exploitability: Moderate
Block edits for devices in inactive organizations.
For non-recovery requests, require
obj.organization.is_activeinhas_change_permission(). The superclass checks managed-organization membership but not organization activity, so a manager can edit devices after the organization is disabled.🤖 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_controller/config/admin.py` around lines 603 - 607, Update has_change_permission() so that, for non-recovery requests with an object, permission requires both the superclass result, an active organization via obj.organization.is_active, and the existing deactivation check. Preserve the current early return for missing objects or recovery views.Source: Path instructions
🤖 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_controller/geo/admin.py`:
- Around line 50-56: In get_form, narrow the broad KeyError handler to only the
expected missing-location-field case, allowing unrelated form-construction
KeyErrors to propagate. Preserve the DjangoModelAdmin fallback and request.user
assignment for read-only forms, and add coverage for both normal form creation
and the read-only fallback path.
---
Outside diff comments:
In `@openwisp_controller/config/admin.py`:
- Around line 603-607: Update has_change_permission() so that, for non-recovery
requests with an object, permission requires both the superclass result, an
active organization via obj.organization.is_active, and the existing
deactivation check. Preserve the current early return for missing objects or
recovery views.
🪄 Autofix
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: e1929e79-fd11-4306-b5a9-c51b57e641db
📒 Files selected for processing (4)
openwisp_controller/config/admin.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/pki/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside itIn Django pull requests, mark all user-facing strings as translatable using the Django internationalization framework.
Files:
openwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.py
**/*
⚙️ CodeRabbit configuration file
**/*: - Flag potential security vulnerabilities
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Flag unused or redundant code
Flag outdated or incorrect comments/docstrings
Ensure new code handles errors properly:
- Log errors that cannot be resolved by the user with error level
- Log unusual conditions with warning level
- Log important background actions with info level
- Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.py
**/*tests*/**
⚙️ CodeRabbit configuration file
**/*tests*/**: Ensure tests cover relevant success, error, boundary, and unusual
input scenarios.Flag tests that depend on arbitrary sleeps, uncontrolled system time,
specific timezones, unseeded randomness, network access, external
services, execution order, shared mutable state, hardcoded ports, or
asynchronous operations that are not properly awaited.
Files:
openwisp_controller/pki/tests/test_api.py
🧠 Learnings (9)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/pki/tests/test_api.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/pki/tests/test_api.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/pki/tests/test_api.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/pki/tests/test_api.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/pki/tests/test_api.py
🔇 Additional comments (6)
openwisp_controller/pki/tests/test_api.py (2)
250-263: Retain the certificate-state assertions from the previous review.
Cert.objects.count()detects deletion only. A revoke or renew implementation can changerevokedorserial_number, and this test still passes. Capture the serial and revocation state before each request, refreshcert, and assert that both remain unchanged after each403response.Source: Path instructions
8-8: LGTM!Also applies to: 22-22, 127-127, 164-205, 264-279
openwisp_controller/config/admin.py (2)
729-731: LGTM!Also applies to: 742-748, 879-898, 1026-1026, 1211-1223, 1277-1281
1006-1017: 🔒 Security & PrivacyProtect activation for disabled organizations.
Ensure
act_deact_device_formrejects activation whendevice.organization.is_activeis false.show_activateonly hides the button, and theactivate_deviceaction filter does not cover this POST path.openwisp_controller/geo/estimated_location/tasks.py (1)
27-47: 🔒 Security & PrivacyMake the organization-state check atomic with the write.
select_for_update(of=("self",))does not lock theOrganizationrow. IfEstimatedLocationService.update_from_whois()lacks its own active-organization guard, deactivation can commit between the check and the write. Lock the organization row or enforce the active predicate at the write.openwisp_controller/geo/admin.py (1)
3-3: LGTM!
9621f79 to
74fdb90
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
openwisp_controller/pki/admin.py (1)
58-61: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-693)
Reachability: External
Allow certificate revocation for disabled organizations.
revoke_actionfilters disabled-organization certificates before calling the inherited revocation action. A privileged admin cannot revoke a compromised certificate after its organization is disabled. The certificate can remain valid.Keep the disabled-organization filter for renewal. Pass the original queryset to
super().revoke_action().Based on PR objectives: “Allow necessary read-only and cleanup operations, including ... certificate revocation cleanup.”
#!/bin/bash set -euo pipefail ast-grep outline openwisp_controller/pki/admin.py --items all fd 'test_admin.py' openwisp_controller/pki --type f \ --exec rg -n -C 5 'revoke_action|renew_cert|disabled.*organization' {}🤖 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_controller/pki/admin.py` around lines 58 - 61, Update revoke_action in the certificate admin to pass the original queryset directly to super().revoke_action(), removing the disabled-organization filtering from revocation; retain that filter in the renewal 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_controller/config/tests/test_handlers.py`:
- Around line 47-49: Update the test around org.save() to patch
openwisp_controller.config.handlers.chain, preventing the transaction callback
from dispatching the real Celery chain; keep the patch active through the save,
then invoke deactivate_organization_devices directly and preserve the existing
assertions.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1061-1071: Update test_fetch_details_skips_when_org_disabled so
the existing WHOISInfo record is stale after device setup, or otherwise prevent
the creation-time lookup, ensuring fetch_whois_details reaches the not
device.organization.is_active guard before asserting mock_client was not called.
In `@openwisp_controller/subnet_division/tests/test_models.py`:
- Around line 683-698: Strengthen the disabled-organization test in
openwisp_controller/subnet_division/tests/test_models.py lines 683-698 by
mocking rule.rule_class.provision_for_existing_objects and asserting it is not
called after the task runs. In
openwisp_controller/geo/estimated_location/tests/tests.py lines 1029-1040, mock
EstimatedLocationService.update_from_whois and assert it is not called for the
skip scenario; retain the existing log assertions.
In `@openwisp_controller/tests/test_users_integration.py`:
- Around line 14-17: Update _get_disabled_org_test_excluded_inline to return the
modified inlines list after appending OrganizationLimitsInline, preserving the
exclusions inherited from the superclass.
---
Duplicate comments:
In `@openwisp_controller/pki/admin.py`:
- Around line 58-61: Update revoke_action in the certificate admin to pass the
original queryset directly to super().revoke_action(), removing the
disabled-organization filtering from revocation; retain that filter in the
renewal flow.
🪄 Autofix
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: 024038dd-9c6b-44b7-84d0-335626f585c0
📒 Files selected for processing (32)
.github/workflows/ci.ymlopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/views.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/admin.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/admin.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/tests/test_users_integration.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside itIn Django pull requests, mark all user-facing strings as translatable using the Django internationalization framework.
Files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/pki/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
**/*
⚙️ CodeRabbit configuration file
**/*: - Flag potential security vulnerabilities
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Flag unused or redundant code
Flag outdated or incorrect comments/docstrings
Ensure new code handles errors properly:
- Log errors that cannot be resolved by the user with error level
- Log unusual conditions with warning level
- Log important background actions with info level
- Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/pki/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
**/*tests*/**
⚙️ CodeRabbit configuration file
**/*tests*/**: Ensure tests cover relevant success, error, boundary, and unusual
input scenarios.Flag tests that depend on arbitrary sleeps, uncontrolled system time,
specific timezones, unseeded randomness, network access, external
services, execution order, shared mutable state, hardcoded ports, or
asynchronous operations that are not properly awaited.
Files:
openwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
.github/**
⚙️ CodeRabbit configuration file
.github/**: Do not complain about dependencies installed from controlled mutable
OpenWISP branches. Branch protection restricts changes to those
branches.
Files:
.github/workflows/ci.yml
🧠 Learnings (11)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/pki/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/pki/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/api/serializers.pyopenwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/api/views.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/config/controller/views.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/exportable.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/pki/admin.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/tests/test_users_integration.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/pki/tests/test_admin.pyopenwisp_controller/connection/tests/test_admin.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/geo/tests/test_admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/tests/test_handlers.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
🪛 ast-grep (0.45.0)
openwisp_controller/config/tasks.py
[warning] 235-235: 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("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/connection/tests/test_api.py
[info] 194-194: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 OpenGrep (1.26.0)
openwisp_controller/config/admin.py
[WARNING] 888-891: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (30)
openwisp_controller/geo/tests/test_admin.py (1)
3-6: LGTM!Also applies to: 16-16, 134-178, 206-238, 321-355, 393-434
openwisp_controller/config/handlers.py (1)
1-1: LGTM!Also applies to: 193-196, 207-215
openwisp_controller/config/tasks.py (1)
129-138: LGTM!Also applies to: 227-249
openwisp_controller/config/tests/test_vpn.py (1)
820-832: LGTM!openwisp_controller/config/exportable.py (1)
4-5: LGTM!Also applies to: 15-15, 125-134
openwisp_controller/connection/api/serializers.py (1)
24-24: LGTM!openwisp_controller/pki/admin.py (1)
1-4: LGTM!Also applies to: 17-56
.github/workflows/ci.yml (1)
75-75: LGTM!openwisp_controller/geo/estimated_location/tasks.py (1)
29-36: LGTM!openwisp_controller/subnet_division/tasks.py (1)
137-150: LGTM!openwisp_controller/config/admin.py (1)
604-604: LGTM!Also applies to: 729-748, 879-898, 1006-1026, 1211-1223, 1277-1283
openwisp_controller/config/api/serializers.py (1)
115-123: LGTM!openwisp_controller/config/controller/views.py (1)
420-421: LGTM!openwisp_controller/connection/api/views.py (1)
37-45: LGTM!Also applies to: 116-116, 132-134
openwisp_controller/config/api/views.py (1)
101-101: LGTM!Also applies to: 157-157, 171-171, 193-193
openwisp_controller/geo/api/views.py (1)
111-111: LGTM!Also applies to: 127-129, 169-181, 216-218, 298-300, 321-321, 330-330
openwisp_controller/mixins.py (1)
3-7: LGTM!Also applies to: 35-35
openwisp_controller/geo/admin.py (1)
3-3: LGTM!Also applies to: 50-59
openwisp_controller/config/whois/tasks.py (1)
68-68: LGTM!Also applies to: 77-77, 92-101
openwisp_controller/geo/estimated_location/tests/tests.py (1)
732-732: LGTM!openwisp_controller/pki/tests/test_admin.py (1)
148-177: LGTM!Also applies to: 179-203, 205-231
openwisp_controller/config/tests/test_controller.py (2)
1224-1240: Exercise the disabled-device organization guard.
_get_reregistration_payloaduses the disabled organization's shared secret. Secret validation runs beforeDeviceRegisterView.post(), so this request is rejected before it reachesdevice.organization.is_active. Use a separate active organization's shared secret while the device remains in the disabled organization.
81-91: LGTM!Also applies to: 1439-1469
openwisp_controller/config/tests/test_admin.py (1)
122-140: LGTM!Also applies to: 688-726, 1006-1101, 2446-2446
openwisp_controller/config/tests/test_api.py (1)
12-20: LGTM!Also applies to: 112-112, 243-259, 276-279, 425-441, 614-715
openwisp_controller/connection/tests/test_admin.py (1)
4-4: LGTM!Also applies to: 15-15, 102-120, 260-277
openwisp_controller/connection/tests/test_api.py (2)
15-15: LGTM!Also applies to: 29-31, 200-216, 459-487, 558-558, 576-576, 622-655
188-199: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorization Bypass (CWE-862): Missing Authorization
Reachability: External
Block command creation for disabled organizations.
This authenticated API request targets a device in a disabled organization, but the test expects HTTP 201. Command creation is a mutating controller operation and can queue a command for that disabled device. Expect HTTP 403 and assert that no command is created.
openwisp_controller/geo/tests/test_api.py (1)
23-23: LGTM!Also applies to: 203-222, 418-419, 548-583, 712-774, 800-800, 862-862, 957-957, 980-998, 1014-1014, 1081-1081, 1098-1098, 1121-1121
openwisp_controller/pki/tests/test_api.py (1)
8-8: LGTM!Also applies to: 22-22, 171-205, 250-287
| org.is_active = False | ||
| org.save() | ||
| tasks.deactivate_organization_devices(org.id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent the real Celery chain from running in this unit test.
org.save() schedules chain(...).delay() through transaction.on_commit. This TransactionTestCase does not mock or await that chain. The task can run concurrently with the direct call on line 49.
Patch openwisp_controller.config.handlers.chain around org.save(), as the partial-failure test does. Then invoke deactivate_organization_devices directly for this test.
Proposed fix
with self.subTest("Devices are deactivated when org gets disabled"):
- org.is_active = False
- org.save()
+ with patch("openwisp_controller.config.handlers.chain"):
+ org.is_active = False
+ org.save()
tasks.deactivate_organization_devices(org.id)As per path instructions: “Flag tests that depend on ... asynchronous operations that are not properly awaited.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| org.is_active = False | |
| org.save() | |
| tasks.deactivate_organization_devices(org.id) | |
| with self.subTest("Devices are deactivated when org gets disabled"): | |
| with patch("openwisp_controller.config.handlers.chain"): | |
| org.is_active = False | |
| org.save() | |
| tasks.deactivate_organization_devices(org.id) |
🤖 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_controller/config/tests/test_handlers.py` around lines 47 - 49,
Update the test around org.save() to patch
openwisp_controller.config.handlers.chain, preventing the transaction callback
from dispatching the real Celery chain; keep the patch active through the save,
then invoke deactivate_organization_devices directly and preserve the existing
assertions.
Source: Path instructions
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||
| @mock.patch(_WHOIS_GEOIP_CLIENT) | ||
| def test_fetch_details_skips_when_org_disabled(self, mock_client): | ||
| whois_obj = self._create_whois_info(ip_address="8.8.8.8") | ||
| device = self._create_device(last_ip=whois_obj.ip_address) | ||
| device.organization.is_active = False | ||
| device.organization.save(update_fields=["is_active"]) | ||
| fetch_whois_details( | ||
| device_pk=device.pk, initial_ip_address=whois_obj.ip_address | ||
| ) | ||
| mock_client.assert_not_called() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make this test reach the disabled-organization guard.
Lines 1064-1065 create a current WHOISInfo record. fetch_whois_details can return at its fresh-record check before it evaluates whether the organization is disabled. The GeoIP assertion then passes for an active organization too.
Mark the record stale after device setup, or use no existing record while preventing the creation-time lookup. This makes the test prove that not device.organization.is_active blocks the GeoIP request.
Proposed test adjustment
whois_obj = self._create_whois_info(ip_address="8.8.8.8")
device = self._create_device(last_ip=whois_obj.ip_address)
+ WHOISInfo.objects.filter(pk=whois_obj.pk).update(
+ modified=timezone.now()
+ - timedelta(days=app_settings.WHOIS_REFRESH_THRESHOLD_DAYS + 1)
+ )
device.organization.is_active = FalseAs per path instructions, tests must cover relevant success, error, boundary, and unusual input scenarios.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | |
| @mock.patch(_WHOIS_GEOIP_CLIENT) | |
| def test_fetch_details_skips_when_org_disabled(self, mock_client): | |
| whois_obj = self._create_whois_info(ip_address="8.8.8.8") | |
| device = self._create_device(last_ip=whois_obj.ip_address) | |
| device.organization.is_active = False | |
| device.organization.save(update_fields=["is_active"]) | |
| fetch_whois_details( | |
| device_pk=device.pk, initial_ip_address=whois_obj.ip_address | |
| ) | |
| mock_client.assert_not_called() | |
| `@mock.patch.object`(app_settings, "WHOIS_CONFIGURED", True) | |
| `@mock.patch`(_WHOIS_GEOIP_CLIENT) | |
| def test_fetch_details_skips_when_org_disabled(self, mock_client): | |
| whois_obj = self._create_whois_info(ip_address="8.8.8.8") | |
| device = self._create_device(last_ip=whois_obj.ip_address) | |
| WHOISInfo.objects.filter(pk=whois_obj.pk).update( | |
| modified=timezone.now() | |
| - timedelta(days=app_settings.WHOIS_REFRESH_THRESHOLD_DAYS + 1) | |
| ) | |
| device.organization.is_active = False | |
| device.organization.save(update_fields=["is_active"]) | |
| fetch_whois_details( | |
| device_pk=device.pk, initial_ip_address=whois_obj.ip_address | |
| ) | |
| mock_client.assert_not_called() |
🤖 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_controller/config/whois/tests/tests.py` around lines 1061 - 1071,
Update test_fetch_details_skips_when_org_disabled so the existing WHOISInfo
record is stale after device setup, or otherwise prevent the creation-time
lookup, ensuring fetch_whois_details reaches the not
device.organization.is_active guard before asserting mock_client was not called.
Source: Path instructions
| def test_provision_subnet_ip_skips_disabled_org(self): | ||
| org = self._create_org(name="disabled-org", slug="disabled-org") | ||
| rule = self._get_vpn_subdivision_rule( | ||
| organization=org, | ||
| master_subnet=self._get_master_subnet( | ||
| subnet="10.200.0.0/16", organization=org | ||
| ), | ||
| ) | ||
| org.is_active = False | ||
| org.save(update_fields=["is_active"]) | ||
| with patch("openwisp_controller.subnet_division.tasks.logger.info") as mocked: | ||
| tasks.provision_subnet_ip_for_existing_devices.run(rule.id) | ||
| mocked.assert_called_once_with( | ||
| "Skipping subnet provisioning for rule %s of disabled organization", | ||
| rule.id, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the protected operation is not called.
Both tests can pass if a later change logs the skip message but continues with the mutation.
openwisp_controller/subnet_division/tests/test_models.py#L683-L698: mockrule.rule_class.provision_for_existing_objectsand assert that it is not called.openwisp_controller/geo/estimated_location/tests/tests.py#L1029-L1040: mockEstimatedLocationService.update_from_whoisand assert that it is not called.
As per path instructions, tests must cover relevant success, error, boundary, and unusual input scenarios.
📍 Affects 2 files
openwisp_controller/subnet_division/tests/test_models.py#L683-L698(this comment)openwisp_controller/geo/estimated_location/tests/tests.py#L1029-L1040
🤖 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_controller/subnet_division/tests/test_models.py` around lines 683 -
698, Strengthen the disabled-organization test in
openwisp_controller/subnet_division/tests/test_models.py lines 683-698 by
mocking rule.rule_class.provision_for_existing_objects and asserting it is not
called after the task runs. In
openwisp_controller/geo/estimated_location/tests/tests.py lines 1029-1040, mock
EstimatedLocationService.update_from_whois and assert it is not called for the
skip scenario; retain the existing log assertions.
Source: Path instructions
| def _get_disabled_org_test_excluded_inline(self): | ||
| inlines = super()._get_disabled_org_test_excluded_inline() | ||
| inlines += [OrganizationLimitsInline] | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the updated exclusion list.
This override ends without returning inlines. The caller receives None, so OrganizationLimitsInline is not excluded and the disabled-organization inline test can fail.
Proposed fix
def _get_disabled_org_test_excluded_inline(self):
inlines = super()._get_disabled_org_test_excluded_inline()
inlines += [OrganizationLimitsInline]
+ return inlines📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _get_disabled_org_test_excluded_inline(self): | |
| inlines = super()._get_disabled_org_test_excluded_inline() | |
| inlines += [OrganizationLimitsInline] | |
| def _get_disabled_org_test_excluded_inline(self): | |
| inlines = super()._get_disabled_org_test_excluded_inline() | |
| inlines += [OrganizationLimitsInline] | |
| return inlines |
🤖 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_controller/tests/test_users_integration.py` around lines 14 - 17,
Update _get_disabled_org_test_excluded_inline to return the modified inlines
list after appending OrganizationLimitsInline, preserving the exclusions
inherited from the superclass.
1 participant
Checklist
Reference to Existing Issue
Closes #1393.
Description of Changes
REST APIs, controller registration re-checks, and background tasks.
is disabled. Re-enabling the organization does not reactivate devices.
Blockers
Screenshot
Disabled Organization Admin
Screencast.from.08-05-26.18.21.17.webm
Disabled Device Admin
Screencast.from.08-05-26.18.21.42.webm
Activating a device in disabled organization
Screencast.from.08-05-26.18.24.14.webm