[fix] Block deleting a certificate or CA in use by a VPN #1419#1432
[fix] Block deleting a certificate or CA in use by a VPN #1419#1432omlahore wants to merge 1 commit into
Conversation
Deleting a Cert or CA that is still assigned to a VPN client cascaded the deletion and left the device configuration corrupted (status stuck at 'applied' with unresolved cert context). CertAdmin/CaAdmin now flag such objects as protected in get_deleted_objects, so the admin blocks the deletion and tells the user to remove the VPN template from the device first. Signed-off-by: Om <omlahore47@gmail.com>
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Nice work on this fix. Blocking the delete at the admin layer via Files Reviewed (3 files)
Reviewed by claude-opus-4.8 · Input: 30 · Output: 3.4K · Cached: 906.3K |
📝 WalkthroughWalkthroughThe PKI admin now blocks deletion of CAs and certificates referenced by VPN servers or clients, using translated protection messages. A regression test covers certificates created through VPN client templates and verifies they appear as protected during admin deletion. The pull request template adds an Anti AI Spam Policy checklist item, expands checklist guidance, clarifies issue references, and introduces description and screenshot sections. Estimated code review effort: 3 (Moderate) | ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/pull_request_template.md:
- Around line 13-15: Clarify in the pull request template that the
“<issue-number>” placeholder in “Closes #<issue-number>.” must be replaced with
the actual issue number, using an explicit instruction near that line.
- Line 6: Update the contributing-guidelines link in the PR template to use the
canonical HTTPS URL directly instead of HTTP, while preserving the existing link
text and checkbox.
In `@openwisp_controller/config/tests/test_admin.py`:
- Around line 1730-1761: Expand test coverage beyond
test_delete_cert_in_use_is_blocked by adding cases for certificates assigned
directly to VPN servers and CAs used by both VPN servers and VPN clients,
asserting the corresponding admin deletion protection messages via
CertAdmin.get_deleted_objects and CaAdmin.get_deleted_objects. Inspect pki tests
for existing normal certificate/CA deletion tests and add coverage only where
absent.
In `@openwisp_controller/pki/admin.py`:
- Around line 15-63: Update _cert_in_use_message and _ca_in_use_message to
collect all matching VpnClient devices and Vpn servers instead of calling
.first(). Build protection messages containing every affected device or VPN,
while preserving the existing single-item wording where appropriate and ensuring
duplicates are removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f45dc61-0a22-4b80-a0bc-2b9a6ccaabb5
📒 Files selected for processing (3)
.github/pull_request_template.mdopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/pki/admin.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | 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.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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 it
Files:
openwisp_controller/pki/admin.pyopenwisp_controller/config/tests/test_admin.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/pki/admin.pyopenwisp_controller/config/tests/test_admin.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/pki/admin.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/pki/admin.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/pki/admin.pyopenwisp_controller/config/tests/test_admin.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/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/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/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/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/tests/test_admin.py
🪛 ast-grep (0.44.1)
openwisp_controller/pki/admin.py
[warning] 21-21: 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", "VpnClient")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 22-22: 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", "Vpn")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 44-44: 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", "VpnClient")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 45-45: 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", "Vpn")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🔇 Additional comments (1)
openwisp_controller/pki/admin.py (1)
2-2: LGTM!Also applies to: 66-79, 93-102
| <!-- Pull requests not following the rules will be closed without further inspection. --> | ||
| <!-- Replace `[ ]` with `N/A` if the requirement is not applicable. --> | ||
|
|
||
| - [ ] I have read the [OpenWISP Contributing Guidelines](http://openwisp.io/docs/developer/contributing.html) and [OpenWISP Anti AI Spam Policy](https://openwisp.io/docs/dev/general/code-of-conduct.html#anti-ai-spam-policy). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Link directly to HTTPS documentation.
The contributing-guidelines link currently uses HTTP and redirects to HTTPS. Use the canonical HTTPS URL directly to avoid the insecure first hop. (openwisp.io)
Proposed fix
-- [ ] I have read the [OpenWISP Contributing Guidelines](http://openwisp.io/docs/developer/contributing.html) and [OpenWISP Anti AI Spam Policy](https://openwisp.io/docs/dev/general/code-of-conduct.html#anti-ai-spam-policy).
+- [ ] I have read the [OpenWISP Contributing Guidelines](https://openwisp.io/docs/stable/developer/contributing.html) and [OpenWISP Anti AI Spam Policy](https://openwisp.io/docs/dev/general/code-of-conduct.html#anti-ai-spam-policy).📝 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.
| - [ ] I have read the [OpenWISP Contributing Guidelines](http://openwisp.io/docs/developer/contributing.html) and [OpenWISP Anti AI Spam Policy](https://openwisp.io/docs/dev/general/code-of-conduct.html#anti-ai-spam-policy). | |
| - [ ] I have read the [OpenWISP Contributing Guidelines](https://openwisp.io/docs/stable/developer/contributing.html) and [OpenWISP Anti AI Spam Policy](https://openwisp.io/docs/dev/general/code-of-conduct.html#anti-ai-spam-policy). |
🤖 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 @.github/pull_request_template.md at line 6, Update the
contributing-guidelines link in the PR template to use the canonical HTTPS URL
directly instead of HTTP, while preserving the existing link text and checkbox.
| <!-- Please [open a new issue](https://github.com/openwisp/openwisp-controller/issues/new/choose) if there isn't an existing issue yet. --> | ||
|
|
||
| Please [open a new issue](https://github.com/openwisp/openwisp-controller/issues/new/choose) if there isn't an existing issue yet. | ||
| Closes #<issue-number>. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that the issue placeholder must be replaced.
If <issue-number> is left unchanged, the generated PR description will not close or reference the intended issue. Add an explicit replacement instruction.
Proposed fix
<!-- Please [open a new issue](https://github.com/openwisp/openwisp-controller/issues/new/choose) if there isn't an existing issue yet. -->
+<!-- Replace <issue-number> with the actual issue number before submitting. -->
Closes #<issue-number>.📝 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.
| <!-- Please [open a new issue](https://github.com/openwisp/openwisp-controller/issues/new/choose) if there isn't an existing issue yet. --> | |
| Please [open a new issue](https://github.com/openwisp/openwisp-controller/issues/new/choose) if there isn't an existing issue yet. | |
| Closes #<issue-number>. | |
| <!-- Please [open a new issue](https://github.com/openwisp/openwisp-controller/issues/new/choose) if there isn't an existing issue yet. --> | |
| <!-- Replace <issue-number> with the actual issue number before submitting. --> | |
| Closes #<issue-number>. |
🤖 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 @.github/pull_request_template.md around lines 13 - 15, Clarify in the pull
request template that the “<issue-number>” placeholder in “Closes
#<issue-number>.” must be replaced with the actual issue number, using an
explicit instruction near that line.
| def test_delete_cert_in_use_is_blocked(self): | ||
| # regression test for issue #1419: deleting a certificate that is | ||
| # still assigned to a VPN client must be blocked from the admin, | ||
| # otherwise the device configuration is left corrupted. | ||
| from django.contrib.admin.sites import site as admin_site | ||
| from django.test import RequestFactory | ||
|
|
||
| org = self._get_org() | ||
| vpn = self._create_vpn() | ||
| config = self._create_config(organization=org) | ||
| vpn_template = self._create_template( | ||
| name="vpn-1419", type="vpn", vpn=vpn, auto_cert=True | ||
| ) | ||
| config.templates.add(vpn_template) | ||
| vpnclient = config.vpnclient_set.first() | ||
| cert = vpnclient.cert | ||
| self.assertIsNotNone(cert) | ||
|
|
||
| User = get_user_model() | ||
| try: | ||
| admin = User.objects.get(username="admin") | ||
| except User.DoesNotExist: | ||
| admin = User.objects.create_superuser("admin", "admin@test.org", "tester") | ||
| request = RequestFactory().get("/") | ||
| request.user = admin | ||
| cert_admin = admin_site._registry[Cert] | ||
| _, _, _, protected = cert_admin.get_deleted_objects([cert], request) | ||
| self.assertTrue( | ||
| any("remove the VPN template" in str(p) for p in protected), | ||
| f"cert in use should be protected from deletion, got: {protected}", | ||
| ) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Test only covers the VPN client certificate path; missing VPN server and CA coverage.
The test verifies cert deletion protection via VPN client, but two other protection paths are untested in this file:
- Certificate used directly by a VPN server (
_cert_in_use_messagelines 34-39) - CA protection via
CaAdmin.get_deleted_objects(both VPN server and VPN client cases)
Consider adding test cases for these paths. Also, verify whether normal (unprotected) deletion tests exist in pki/tests/.
#!/bin/bash
# Check for existing CA deletion protection and normal deletion tests
rg -n "get_deleted_objects\|_ca_in_use\|_cert_in_use\|delete.*cert\|delete.*ca" \
openwisp_controller/pki/tests/ --type=py -C3🤖 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_admin.py` around lines 1730 - 1761,
Expand test coverage beyond test_delete_cert_in_use_is_blocked by adding cases
for certificates assigned directly to VPN servers and CAs used by both VPN
servers and VPN clients, asserting the corresponding admin deletion protection
messages via CertAdmin.get_deleted_objects and CaAdmin.get_deleted_objects.
Inspect pki tests for existing normal certificate/CA deletion tests and add
coverage only where absent.
| def _cert_in_use_message(cert): | ||
| """Returns a message if ``cert`` is in use by a VPN, otherwise ``None``. | ||
|
|
||
| Deleting a certificate that is still assigned to a VPN client cascades | ||
| the deletion of the client and leaves the device configuration in a | ||
| corrupted state (issue #1419), so deletion is blocked from the admin. | ||
| """ | ||
| VpnClient = load_model("config", "VpnClient") | ||
| Vpn = load_model("config", "Vpn") | ||
| vpn_client = ( | ||
| VpnClient.objects.filter(cert=cert).select_related("config__device").first() | ||
| ) | ||
| if vpn_client: | ||
| device = vpn_client.config.device | ||
| return _( | ||
| 'the certificate "%(cert)s" is currently used by the device ' | ||
| '"%(device)s"; remove the VPN template from the device before ' | ||
| "deleting this certificate." | ||
| ) % {"cert": cert, "device": device} | ||
| vpn = Vpn.objects.filter(cert=cert).first() | ||
| if vpn: | ||
| return _( | ||
| 'the certificate "%(cert)s" is currently used by the VPN server ' | ||
| '"%(vpn)s"; change the VPN certificate before deleting this one.' | ||
| ) % {"cert": cert, "vpn": vpn} | ||
| return None | ||
|
|
||
|
|
||
| def _ca_in_use_message(ca): | ||
| """Returns a message if ``ca`` is in use by a VPN, otherwise ``None``.""" | ||
| VpnClient = load_model("config", "VpnClient") | ||
| Vpn = load_model("config", "Vpn") | ||
| vpn = Vpn.objects.filter(ca=ca).first() | ||
| if vpn: | ||
| return _( | ||
| 'the CA "%(ca)s" is currently used by the VPN server "%(vpn)s"; ' | ||
| "change the VPN CA before deleting this one." | ||
| ) % {"ca": ca, "vpn": vpn} | ||
| vpn_client = ( | ||
| VpnClient.objects.filter(cert__ca=ca).select_related("config__device").first() | ||
| ) | ||
| if vpn_client: | ||
| device = vpn_client.config.device | ||
| return _( | ||
| 'the CA "%(ca)s" issued a certificate used by the device ' | ||
| '"%(device)s"; remove the VPN template from the device before ' | ||
| "deleting this CA." | ||
| ) % {"ca": ca, "device": device} | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only the first affected device/VPN is reported in protection messages.
Using .first() means only one VPN client or VPN server is identified when multiple devices or VPNs reference the same cert/CA. For a CA with many issued certificates in use by VPN clients, the admin must retry deletion after each device's template removal, seeing one device at a time.
Consider listing all affected devices/VPNs in the protection message.
♻️ Proposed fix: list all affected devices/VPNs
def _cert_in_use_message(cert):
"""Returns a message if ``cert`` is in use by a VPN, otherwise ``None``.
Deleting a certificate that is still assigned to a VPN client cascades
the deletion of the client and leaves the device configuration in a
corrupted state (issue `#1419`), so deletion is blocked from the admin.
"""
VpnClient = load_model("config", "VpnClient")
Vpn = load_model("config", "Vpn")
- vpn_client = (
- VpnClient.objects.filter(cert=cert).select_related("config__device").first()
- )
- if vpn_client:
- device = vpn_client.config.device
+ vpn_clients = VpnClient.objects.filter(cert=cert).select_related(
+ "config__device"
+ )
+ if vpn_clients:
+ devices = ", ".join(str(vc.config.device) for vc in vpn_clients)
return _(
'the certificate "%(cert)s" is currently used by the device '
'"%(device)s"; remove the VPN template from the device before '
"deleting this certificate."
- ) % {"cert": cert, "device": device}
- vpn = Vpn.objects.filter(cert=cert).first()
+ vpns = Vpn.objects.filter(cert=cert)
+ if vpns:
+ vpn_names = ", ".join(str(v) for v in vpns)
return _(
'the certificate "%(cert)s" is currently used by the VPN server '
'"%(vpn)s"; change the VPN certificate before deleting this one.'
- ) % {"cert": cert, "vpn": vpn}
+ ) % {"cert": cert, "vpn": vpn_names}
return None def _ca_in_use_message(ca):
"""Returns a message if ``ca`` is in use by a VPN, otherwise ``None``."""
VpnClient = load_model("config", "VpnClient")
Vpn = load_model("config", "Vpn")
- vpn = Vpn.objects.filter(ca=ca).first()
+ vpns = Vpn.objects.filter(ca=ca)
if vpn:
return _(
'the CA "%(ca)s" is currently used by the VPN server "%(vpn)s"; '
"change the VPN CA before deleting this one."
- ) % {"ca": ca, "vpn": vpn}
- vpn_client = (
- VpnClient.objects.filter(cert__ca=ca).select_related("config__device").first()
- )
- if vpn_client:
- device = vpn_client.config.device
+ vpn_clients = VpnClient.objects.filter(cert__ca=ca).select_related(
+ "config__device"
+ )
+ if vpn_clients:
+ devices = ", ".join(str(vc.config.device) for vc in vpn_clients)
return _(
'the CA "%(ca)s" issued a certificate used by the device '
'"%(device)s"; remove the VPN template from the device before '
"deleting this CA."
- ) % {"ca": ca, "device": device}
+ ) % {"ca": ca, "device": devices}
return NoneNote: with multiple devices/VPNs the message grammar ("the device") becomes slightly imprecise; a follow-up could use plural-aware translation strings.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 21-21: 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", "VpnClient")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 22-22: 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", "Vpn")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 44-44: 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", "VpnClient")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 45-45: 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", "Vpn")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🤖 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 15 - 63, Update
_cert_in_use_message and _ca_in_use_message to collect all matching VpnClient
devices and Vpn servers instead of calling .first(). Build protection messages
containing every affected device or VPN, while preserving the existing
single-item wording where appropriate and ensuring duplicates are removed.
1 participant
Checklist
Reference to Existing Issue
Closes #1419.
Description of Changes
Deleting a certificate or CA that is still assigned to a VPN client cascaded the deletion and left the device configuration in a corrupted state: the config stayed stuck at status
appliedwhile its cert context could no longer be resolved.This change blocks that deletion at the admin layer instead of letting it cascade.
CertAdminandCaAdminnow overrideget_deleted_objectsand mark any certificate or CA that is still referenced by aVpnClientas protected.delete_selectedaction), Django surfaces it under "protected related objects" and refuses, telling the user which VPN clients still use it.For a CA, the check also covers certificates issued under it that are in use, so you cannot orphan an in-use cert by deleting its CA.
Tests
Added tests in
config/tests/test_admin.pythat:VpnClientreferences the object (fails without this change),