[feature] Added X.509 Certificate Generator Templates - #1378
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds standalone Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 review |
✅ Actions performedReview triggered.
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous Review Summary (commit 1f6fe30)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1f6fe30)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Reviewed by step-3.7-flash · Input: 128.2K · Output: 35.6K · Cached: 2.8M |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/base/template.py`:
- Around line 265-267: The help text for the auto_cert field is out of date (it
still says it's only valid for VPN templates) — update the auto_cert field's
help/verbose/help_text in the Template definition in
openwisp_controller/config/base/template.py so it matches the new behavior
(auto_cert is allowed when type == "cert" as well as when type == "vpn"); locate
the auto_cert attribute (and any admin/API serializer or form label/help_text
referencing it) and change the message to something like "Valid for 'vpn' and
'cert' template types" or equivalent clear wording that includes both types.
🪄 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: 33bb61f8-c083-446e-8e45-44d753e7ff7b
📒 Files selected for processing (3)
openwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle 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, and provide user-facing messages for errors that the user can solve autonomously
Files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
🧠 Learnings (4)
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.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:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
📚 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/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (3)
openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 62-83, 251-253, 271-312
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
1-59: LGTM!tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
1-58: LGTM!
{
"failures": [
{
"type": "Build/Infrastructure/Other",
"message": "Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue!",
"details": "The CI environment detected that the database migrations are out of sync. This is indicated by the error message 'Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue!' and the mention of a specific migration file '0005_user_expiration_date_user_user_active_expiry_idx.py'."
}
]
}CI Failure AnalysisHello @stktyagi,
Fix: Run |
Updated previous tests and added new tests for implemetation. Fixes #1356
Fixed help text for auto cert and updated migration files
Migrations Check FailedHello @stktyagi, The CI failed because there are unapplied database migrations. This means that the database schema is out of sync with the current state of the code. Fix: ./manage.py makemigrations |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/base/template.py`:
- Around line 252-253: The validation of certificate relations (calls to
self._validate_org_relation("ca") and
self._validate_org_relation("blueprint_cert")) is running unconditionally and
can fail on stale relations when the object is being switched away from the
"cert" type; restrict these validations to the cert branch so they only run when
the template's type is "cert" (e.g., wrap or move the
self._validate_org_relation(...) calls inside the same conditional that handles
the "cert" branch or after the type check that preserves/clears ca and
blueprint_cert), ensuring they do not run when the code path clears those fields
(see the branch that clears ca and blueprint_cert).
- Around line 304-305: The current check "if not self.config" coerces any falsy
value ([], "", False) into {}, bypassing BaseConfig.clean() validation; change
the condition to only handle missing configs by checking "if self.config is
None" (or equivalent explicit None check) so only absent configs are replaced
with {} and invalid/falsy payloads are left intact for
full_clean()/BaseConfig.clean() to reject.
🪄 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: d3d93328-e58d-41dd-a374-dffebd6d1e38
📒 Files selected for processing (5)
openwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle 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, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
🧠 Learnings (4)
📚 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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.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/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (4)
openwisp_controller/pki/tests/test_api.py (1)
155-155: LGTM!Also applies to: 275-275
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
12-16: LGTM!Also applies to: 19-44, 45-58, 59-74
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
11-14: LGTM!Also applies to: 17-42, 43-56, 57-72
openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 62-83, 119-120
Migrations Check FailedHello @stktyagi, The CI failed because there are unapplied database migrations. Failure: Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue! Fix: ./manage.py makemigrations |
…s/1356-extend-abstract-template
Validate cert relations only inside the cert branch and Only coerce missing cert configs, not every falsy value. Fixes #1356
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Added test for the validation branch that now skips ca / blueprint_cert checks for non-cert templates Fixes #1356
|
Coverage is 98.321% — issues/1356-extend-abstract-template into gsoc26-x509-certificate-generator-templates. No base build found for |
Fixed line too long flake error Fixes #1356
Updated test by joining the list of strings into one sentence. Fixes #1377
stktyagi
changed the title
| "required", | ||
| "created", | ||
| ] | ||
| multitenant_shared_relations = ("vpn", "ca", "blueprint_cert") |
There was a problem hiding this comment.
Adding these organization-scoped certificate relations exposes an existing clone path that now creates invalid templates. A clone is validated with the source organization, then its organization is changed and saved without validation, so it can keep a non-shared CA or blueprint from the source organization. Please set the target organization before validating and saving the clone, and add coverage for cloning certificate templates across organizations.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
These issues need to be fixed before merge.
| self.auto_cert = False | ||
| if self.type != "cert": | ||
| self.auto_cert = False | ||
| if self.type == "cert" and not self.auto_cert: |
There was a problem hiding this comment.
Certificate template creation through the API stops working when OPENWISP_CONTROLLER_DEFAULT_AUTO_CERT is False. The API does not expose auto_cert, so the default remains false and this validation rejects the request, which is problematic..
Addressed comments included in latest review
Refresh tracked values only for fields that were saved
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_controller/config/base/device.py (2)
354-380: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the blank lines inside these method bodies.
Line 357 and line 378 add blank lines inside the method bodies. The coding guidelines require avoiding unnecessary blank lines inside function and method bodies.
♻️ Proposed cleanup
def _check_name_changed(self, update_fields=None): if self._initial_name == models.DEFERRED: return - field_saved = update_fields is None or "name" in update_fields if field_saved and self._initial_name != self.name: device_name_changed.send( sender=self.__class__, instance=self, ) - if self._has_config(): self.config.set_status_modified() - if field_saved: self._initial_name = self.name def _check_mac_address_changed(self, update_fields=None): if self._initial_mac_address == models.DEFERRED: return field_saved = update_fields is None or "mac_address" in update_fields if field_saved and self._initial_mac_address != self.mac_address: if self._has_config(): self.config.set_status_modified() - if field_saved: self._initial_mac_address = self.mac_address🤖 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/base/device.py` around lines 354 - 380, Remove the unnecessary blank lines within _check_name_changed and _check_mac_address_changed, including those immediately after the deferred-value guards and before the final field-state updates, without changing behavior.Source: Coding guidelines
391-403: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate
management_ip_changedon the fields saved insave().
Management_ipis inDevice._changed_checked_fields, sosave(update_fields=["name"])still calls_check_management_ip_changed()with that sameupdate_fields. The current code emits the signal from the in-memory value even whenmanagement_ipwas not saved. Gate the signal with the sameupdate_fields is None or "management_ip" in update_fieldstest used to advance_initial_management_ip.🤖 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/base/device.py` around lines 391 - 403, The _check_management_ip_changed method currently emits management_ip_changed even when management_ip is excluded from save(update_fields=...). Gate the signal emission with the same update_fields is None or "management_ip" in update_fields condition already used when updating _initial_management_ip, while preserving the deferred-value handling and state update behavior.
♻️ Duplicate comments (2)
openwisp_controller/config/base/template.py (1)
356-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the blank lines inside
_validate_cert_template_changes.Lines 358 and 366 add blank lines inside the method body. The coding guidelines require avoiding unnecessary blank lines inside function and method bodies. A reviewer already raised the same point on this file.
♻️ Proposed change
if not changing_protected_fields: return - Config = load_model("config", "Config") if not ( Config.objects.filter(templates=self) .exclude(status__in=["deactivating", "deactivated"]) .exists() ): return - errors = {}🤖 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/base/template.py` around lines 356 - 366, Remove the unnecessary blank lines inside _validate_cert_template_changes, specifically between the changing_protected_fields guard and Config assignment, and between the Config existence check and the following logic. Keep the method’s behavior unchanged.Source: Coding guidelines
openwisp_controller/config/tests/test_template.py (1)
1565-1586: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test no longer covers the NULL
cert_idcase its name and docstring describe.The docstring states the test verifies that "a DeviceCertificate with cert=None does not poison the get_unassigned_certs() SQL query due to NULL semantics". The body instead lets
manage_device_certsprovision a certificate and then assertsassertIsNotNone(device_cert.cert_id). No row with a NULLcert_idexists.
get_unassigned_certsfilters withcert_id__isnull=Falseprecisely to guard against that NULL case. That guard is now untested: removing thecert_id__isnull=Falsefilter would still let this test pass.Create a row with
cert=Noneand assert the precondition, so the guard is pinned.💚 Proposed fix
config.templates.add(template) device_cert = DeviceCertificate.objects.get(config=config, template=template) self.assertIsNotNone(device_cert.cert_id) + # force a real NULL cert_id to exercise the SQL NULL guard + DeviceCertificate.objects.filter(pk=device_cert.pk).update(cert=None) + device_cert.refresh_from_db() + self.assertIsNone(device_cert.cert_id) choices = get_unassigned_certs() queryset = choices.get("pk__in") self.assertIsNotNone(queryset) self.assertIn(unassigned_cert, queryset)🤖 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_template.py` around lines 1565 - 1586, Update test_get_unassigned_certs_with_null_device_cert to explicitly set the retrieved DeviceCertificate’s cert field to None and save it before calling get_unassigned_certs(). Replace the current assertIsNotNone(device_cert.cert_id) with an assertion that cert_id is None, while preserving the existing assertion that the unassigned certificate remains in the returned queryset.
🤖 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/admin.py`:
- Around line 1157-1160: Update AbstractTemplate.clone() to use a sentinel
default for organization, preserving the source organization only when the
argument is omitted while allowing an explicit None to create a shared clone.
Ensure the cloning action passes the explicit shared target through the existing
organization selection path. Add coverage for the organization == "" action case
and verify the resulting clone has no organization.
In `@openwisp_controller/config/base/template.py`:
- Around line 189-206: Update Template.save to keep kwargs["update_fields"] as
the primary path, while retaining the args[3] handling only for Django 5.1/5.2
compatibility. Add a concise comment above the positional branch documenting
that index 3 is the legacy update_fields argument and is removed in Django 6.0.
- Around line 501-504: Update Template.clone to distinguish an omitted
organization from an explicit request for organization=None, using a distinct
unset sentinel while preserving the current source-organization behavior when
the argument is omitted. Ensure the admin action’s organization == "" path
passes the explicit shared-clone value so superusers can create clones without
an organization.
- Around line 212-221: Update _get_initial_value_or_fallback to resolve deferred
ForeignKey fields using their relation names, while loading the corresponding
*_id column via only(). For fields such as ca and blueprint_cert, fetch the
object using ca_id/blueprint_cert_id, assign the FK value to _initial_{field},
and then return getattr(obj, field); preserve existing handling for non-FK
fields, missing objects, and non-deferred values.
In
`@openwisp_controller/config/templates/admin/config/device_certificates_table.html`:
- Around line 55-57: Update the boolean icon alt attributes in the device
certificates table template to use Django’s translation helper instead of
literal “True” and “False” text, preserving the existing icons and ensuring both
status labels—including the revoked state—are localized.
In `@openwisp_controller/config/tests/test_api.py`:
- Around line 1609-1638: Wrap the PUT request in the test method using
captureOnCommitCallbacks(execute=True), matching the existing pattern in
test_device_api_cert_template_lifecycle, so deferred post_clear cleanup runs
before the subsequent assertions. Keep the current response and
certificate-preservation assertions unchanged.
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 1007-1016: Update test_setting_disabled to mock or spy on
DeviceCertificate.regenerate_certificates and assert it is not called when
REGENERATE_CERTS_ON_HARDWARE_CHANGE is false. Update test_device_does_not_exist
to explicitly assert the task completes without raising an exception, rather
than asserting its always-None return value.
In `@openwisp_controller/config/utils.py`:
- Around line 110-144: Update get_client_extensions and
AbstractVpnClient._auto_create_cert so DEFAULT_CLIENT_EXTENSIONS is deep-copied,
including each mutable extension dictionary, before use. Preserve blueprint
extension copying and hardware OID handling while ensuring every generated
certificate receives independent extension entries.
---
Outside diff comments:
In `@openwisp_controller/config/base/device.py`:
- Around line 354-380: Remove the unnecessary blank lines within
_check_name_changed and _check_mac_address_changed, including those immediately
after the deferred-value guards and before the final field-state updates,
without changing behavior.
- Around line 391-403: The _check_management_ip_changed method currently emits
management_ip_changed even when management_ip is excluded from
save(update_fields=...). Gate the signal emission with the same update_fields is
None or "management_ip" in update_fields condition already used when updating
_initial_management_ip, while preserving the deferred-value handling and state
update behavior.
---
Duplicate comments:
In `@openwisp_controller/config/base/template.py`:
- Around line 356-366: Remove the unnecessary blank lines inside
_validate_cert_template_changes, specifically between the
changing_protected_fields guard and Config assignment, and between the Config
existence check and the following logic. Keep the method’s behavior unchanged.
In `@openwisp_controller/config/tests/test_template.py`:
- Around line 1565-1586: Update test_get_unassigned_certs_with_null_device_cert
to explicitly set the retrieved DeviceCertificate’s cert field to None and save
it before calling get_unassigned_certs(). Replace the current
assertIsNotNone(device_cert.cert_id) with an assertion that cert_id is None,
while preserving the existing assertion that the unassigned certificate remains
in the returned queryset.
🪄 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 Plus
Run ID: 56530c5c-2fb7-489e-a219-2c139a7cada2
⛔ Files ignored due to path filters (1)
openwisp_controller/config/static/config/img/help.svgis excluded by!**/*.svg
📒 Files selected for processing (37)
docs/developer/extending.rstdocs/index.rstdocs/user/certificate-templates.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/settings.rstdocs/user/templates.rstopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/static/config/css/admin.cssopenwisp_controller/config/static/config/js/switcher.jsopenwisp_controller/config/tasks.pyopenwisp_controller/config/templates/admin/config/device/change_form.htmlopenwisp_controller/config/templates/admin/config/device_certificates_table.htmlopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- 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.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.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.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
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/config/settings.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pytests/openwisp2/settings.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/vpn.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_device.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/config/settings.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_device.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/config/settings.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_device.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/config/settings.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/models.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pytests/openwisp2/settings.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/vpn.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_device.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_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.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_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.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_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.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_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.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_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.py
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py
🪛 ast-grep (0.45.0)
openwisp_controller/config/tasks.py
[warning] 224-224: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/handlers.py
[warning] 81-81: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/base/device_certificate.py
[warning] 49-49: 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", "Template")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 232-232: 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)
tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py
[info] 105-105: use help_text to document model columns
Context: models.CharField(blank=True, max_length=64, null=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/config/tests/test_selenium.py
[warning] 25-25: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_notifications", "Notification")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/base/config.py
[warning] 194-194: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/x509_admin.py
[warning] 9-9: 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", "Config")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 10-10: 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)
[warning] 11-11: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/base/template.py
[warning] 40-40: 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("django_x509", "Cert")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 41-41: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 358-358: 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", "Config")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 423-423: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/tests/test_device.py
[warning] 32-32: 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("django_x509", "Cert")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 33-33: 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("django_x509", "Ca")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 34-34: 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", "OrganizationConfigSettings")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 994-994: use jsonify instead of json.dumps for JSON output
Context: json.dumps(expected_cert_ids, default=str)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 1195-1195: 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", "Template")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 HTMLHint (1.9.2)
openwisp_controller/config/templates/admin/config/device_certificates_table.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (65)
docs/developer/extending.rst (1)
345-345: LGTM!docs/index.rst (1)
40-40: LGTM!docs/user/certificate-templates.rst (1)
1-282: LGTM!docs/user/intro.rst (1)
38-39: LGTM!docs/user/rest-api.rst (1)
1115-1115: LGTM!docs/user/settings.rst (1)
311-313: LGTM!Also applies to: 333-349, 362-378
docs/user/templates.rst (1)
212-231: LGTM!tests/openwisp2/sample_config/models.py (2)
99-105: 📐 Maintainability & Code QualityConfirm that the extending guide documents
DeviceCertificate.A reviewer already asked for
docs/developer/extending.rstto list this model. The documentation file is not part of this context, so the update cannot be confirmed here. Extenders need the swappable model name, the settingCONFIG_DEVICECERTIFICATE_MODEL, and the sample-app declaration pattern.#!/bin/bash # Check whether the extending guide covers the new swappable model. set -euo pipefail fd -i 'extending.rst' | while IFS= read -r f; do echo "=== $f ===" rg -n -C3 'DeviceCertificate|DEVICECERTIFICATE|VpnClient' "$f" || echo "no DeviceCertificate references" done
5-5: LGTM!openwisp_controller/config/settings.py (1)
37-39: 📐 Maintainability & Code Quality | ⚡ Quick winDocument the new public setting.
REGENERATE_CERTS_ON_HARDWARE_CHANGEbecomes the public settingOPENWISP_CONTROLLER_REGENERATE_CERTS_ON_HARDWARE_CHANGE. The default isTrue, so an existing deployment revokes and reissues device certificates after any device rename or MAC-address change. Document the setting, the default, and that operational effect indocs/user/settings.rst.A reviewer also asked to correct
OPENWISP_CONTROLLER_COMMON_NAME_FORMATon the same page, because certificate templates now use it in addition to VPN clients.#!/bin/bash # Check documentation coverage for the new and the updated settings. set -euo pipefail fd -i 'settings.rst' | while IFS= read -r f; do echo "=== $f ===" rg -n -C4 'REGENERATE_CERTS_ON_HARDWARE_CHANGE|COMMON_NAME_FORMAT|DEFAULT_AUTO_CERT' "$f" || echo "no matches" doneopenwisp_controller/config/base/template.py (5)
9-9: LGTM!Also applies to: 25-29, 40-51
74-96: LGTM!Also applies to: 133-134
279-288: LGTM!
330-337: LGTM!
443-481: LGTM!openwisp_controller/config/utils.py (1)
1-16: LGTM!Also applies to: 94-107, 147-156
openwisp_controller/config/base/vpn.py (1)
36-41: LGTM!Also applies to: 987-987, 1017-1017, 1041-1042
openwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.py (1)
16-20: LGTM!Also applies to: 23-49, 80-151
openwisp_controller/config/models.py (1)
5-5: LGTM!Also applies to: 97-105
tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py (1)
15-18: LGTM!Also applies to: 21-47, 78-149
tests/openwisp2/settings.py (1)
295-295: LGTM!openwisp_controller/config/api/serializers.py (1)
20-20: LGTM!Also applies to: 42-62, 88-94, 245-245, 343-375
openwisp_controller/config/admin.py (1)
52-65: LGTM!Also applies to: 122-126, 929-932, 1073-1110
openwisp_controller/config/static/config/js/switcher.js (1)
4-60: LGTM!openwisp_controller/config/x509_admin.py (1)
1-126: LGTM!openwisp_controller/config/templates/admin/config/device/change_form.html (1)
13-24: LGTM!openwisp_controller/config/templates/admin/config/device_certificates_table.html (2)
1-54: LGTM!
59-77: LGTM!openwisp_controller/config/static/config/css/admin.css (1)
9-9: LGTM!Also applies to: 409-458
openwisp_controller/config/base/device_certificate.py (4)
1-47: LGTM!
49-141: LGTM!
143-224: LGTM!
226-296: LGTM!openwisp_controller/config/apps.py (1)
168-168: LGTM!Also applies to: 194-198, 220-224
openwisp_controller/config/handlers.py (1)
1-8: LGTM!Also applies to: 46-93
openwisp_controller/config/tasks.py (1)
223-226: LGTM!openwisp_controller/config/tests/test_config.py (1)
878-885: LGTM!openwisp_controller/config/tests/test_vpn.py (1)
558-559: LGTM!Also applies to: 575-576
openwisp_controller/pki/tests/test_api.py (1)
155-156: LGTM!Also applies to: 247-308, 394-419
openwisp_controller/config/base/config.py (5)
67-73: LGTM!
185-198: LGTM!
613-657: LGTM!
1087-1113: LGTM!
1142-1142: LGTM!openwisp_controller/config/base/device.py (3)
33-39: LGTM!
303-305: LGTM!Also applies to: 315-315
324-352: LGTM!openwisp_controller/config/tests/test_admin.py (2)
9-12: LGTM!Also applies to: 38-41, 59-59
1761-1905: LGTM!Also applies to: 2438-2439
openwisp_controller/config/tests/test_api.py (3)
36-36: LGTM!Also applies to: 559-559, 747-747
1340-1525: LGTM!
1527-1563: LGTM!openwisp_controller/config/tests/test_device.py (3)
1-11: LGTM!Also applies to: 32-34, 43-43
748-846: LGTM!
848-1005: LGTM!Also applies to: 1018-1077, 1080-1322
openwisp_controller/config/tests/test_selenium.py (4)
25-26: LGTM!
491-528: LGTM!
846-880: LGTM!
557-561: 📐 Maintainability & Code QualityNo change needed. Celery eager execution is enabled for tests, and the notification text assertion targets the intended message.
openwisp_controller/config/tests/test_template.py (6)
2-9: LGTM!Also applies to: 18-18, 32-32
196-234: LGTM!Also applies to: 633-633
1014-1102: LGTM!
1105-1314: LGTM!
1316-1563: LGTM!
1588-1730: LGTM!
Minor reviews addressed
Case that posts organization= and asserts that the created clone has organization is None.
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
…s/1356-extend-abstract-template
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_template.py (1)
1565-1586: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe docstring and test name no longer match the test body.
The docstring states the test covers "a DeviceCertificate with cert=None". The body now asserts the opposite at line 1582:
self.assertIsNotNone(device_cert.cert_id). No row withcert_id IS NULLis created, so thecert_id__isnull=Falsefilter inget_unassigned_certs()is never exercised. Removing that filter would not fail this test.Create a genuinely NULL row with
auto_cert=Falseso the regression covers the NULL path, and keep the assigned-cert row to also cover exclusion.💚 Proposed fix
def test_get_unassigned_certs_with_null_device_cert(self): """ Test that a DeviceCertificate with cert=None does not poison the get_unassigned_certs() SQL query due to NULL semantics. """ org = self._get_org() ca = self._create_ca(name="Test-CA", organization=org) unassigned_cert = self._create_cert( name="Available-Blueprint", ca=ca, organization=org ) device = self._create_device(name="Test-Device", organization=org) config = self._create_config(device=device) template = self._create_template( name="Test-Template", type="cert", ca=ca, organization=org, config={} ) config.templates.add(template) device_cert = DeviceCertificate.objects.get(config=config, template=template) self.assertIsNotNone(device_cert.cert_id) + # a row with a NULL cert_id must not exclude every unassigned cert + null_template = self._create_template( + name="Null-Template", type="cert", ca=ca, organization=org, config={} + ) + config.templates.add(null_template) + null_device_cert = DeviceCertificate.objects.get( + config=config, template=null_template + ) + DeviceCertificate.objects.filter(pk=null_device_cert.pk).update(cert=None) + null_device_cert.refresh_from_db() + self.assertIsNone(null_device_cert.cert_id) choices = get_unassigned_certs() queryset = choices.get("pk__in") self.assertIsNotNone(queryset) self.assertIn(unassigned_cert, queryset) + self.assertNotIn(device_cert.cert, queryset)🤖 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_template.py` around lines 1565 - 1586, Update test_get_unassigned_certs_with_null_device_cert to create a DeviceCertificate with cert=None and auto_cert=False, ensuring the get_unassigned_certs() NULL-filter path is exercised. Retain the existing assigned certificate row and assertions so the test also verifies assigned certificates remain excluded, and align the test setup/docstring with both cases.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/config/admin.py`:
- Around line 993-996: Replace the hasattr/device.config condition in the
certificate-details logic with the existing DeviceAdmin _has_config() helper,
matching its usage in delete_view, while preserving the current
get_device_certificate_details(device.config) call when configuration is
present.
- Around line 1137-1138: Update TemplateAdmin to define list_select_related for
the ca and blueprint_cert foreign-key relations used by list_display, ensuring
both relations are selected in the template changelist and avoiding per-row
queries.
In `@openwisp_controller/config/api/serializers.py`:
- Line 362: Remove the explicit DeviceCertificate.objects.filter(...).delete()
from the config/template-clearing flow and remove the module-level
DeviceCertificate import if it becomes unused, relying on templates.clear() and
its post_clear reconciliation. Add a regression test covering an organization
change for a device using a shared required certificate template, asserting both
the DeviceCertificate row and certificate ID remain unchanged.
In `@openwisp_controller/config/base/device_certificate.py`:
- Line 163: Remove the unnecessary blank lines inside the _build_cert and
regenerate_certificates method bodies, specifically the lines separating blocks
already divided by explanatory comments. Preserve all code and comments while
keeping the methods’ existing behavior unchanged.
- Around line 82-85: Update both conditions in the DeviceCertificate clean logic
that access self.config.device.organization_id to also require self.config_id,
alongside the existing template_id and cert_id guards, so clean() does not
dereference a missing config.
- Around line 244-248: Update the select_for_update() call in the
active_device_certs query within the regeneration flow to use of=("self",),
limiting row locks to DeviceCertificate while preserving the existing
select_related("cert", "config", "template") and filtering behavior.
In `@openwisp_controller/config/base/device.py`:
- Around line 303-305: Update AbstractDevice.save() to resolve update_fields
from the fourth positional argument (args[3]) when the keyword value is absent,
matching AbstractTemplate.save(). Use this normalized value for the subsequent
field checks so unsaved fields do not trigger side effects.
In `@openwisp_controller/config/base/template.py`:
- Around line 473-478: In the type-handling logic, remove the nested auto_cert
reset under the self.type != "cert" condition and keep one explicit auto_cert
assignment based on self.type == "cert", preserving the existing certificate
behavior.
- Around line 285-294: Update the prefetch logic in the configuration
transaction around device certificates to load both DeviceCertificate and
VpnClient, then derive each reverse relation name from its model’s config field
via config.field.remote_field.get_accessor_name(). Use these derived accessors
in the corresponding Prefetch calls instead of hardcoded devicecertificate_set
or vpnclient_set values.
In
`@openwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.py`:
- Around line 16-20: Add swappable dependencies for settings.CONFIG_CONFIG_MODEL
and settings.CONFIG_TEMPLATE_MODEL to the dependencies list in migration
0066_template_blueprint_cert_template_ca_and_more, alongside the existing Django
X.509 dependencies, so the migration waits for the swapped Config and Template
model tables.
In
`@openwisp_controller/config/templates/admin/config/device_certificates_table.html`:
- Around line 52-62: Update the certificate status rendering in the
cert.has_cert branch so cert.is_revoked uses the negative icon and non-revoked
certificates use the positive icon, preserving the existing “Pending Generation”
output for certificates without a certificate.
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 1330-1334: Update test_post_delete_object_does_not_exist to assert
that dc.cert.revoke was called exactly once with no arguments after invoking
DeviceCertificate.post_delete, preserving the existing ObjectDoesNotExist setup.
In `@openwisp_controller/config/x509_admin.py`:
- Around line 28-40: The __init__ method currently accesses the private
admin_site._registry mapping and can raise KeyError when Device is not
registered there. Resolve the Device admin through the supported get_model_admin
mechanism, following the existing plan in config/admin.py; if compatibility
requires a fallback, guard the lookup and let get_queryset_for_field degrade
safely when no admin is available.
---
Duplicate comments:
In `@openwisp_controller/config/tests/test_template.py`:
- Around line 1565-1586: Update test_get_unassigned_certs_with_null_device_cert
to create a DeviceCertificate with cert=None and auto_cert=False, ensuring the
get_unassigned_certs() NULL-filter path is exercised. Retain the existing
assigned certificate row and assertions so the test also verifies assigned
certificates remain excluded, and align the test setup/docstring with both
cases.
🪄 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 Plus
Run ID: 8109431a-3845-4355-b216-f2532806bcaf
⛔ Files ignored due to path filters (1)
openwisp_controller/config/static/config/img/help.svgis excluded by!**/*.svg
📒 Files selected for processing (37)
docs/developer/extending.rstdocs/index.rstdocs/user/certificate-templates.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/settings.rstdocs/user/templates.rstopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/static/config/css/admin.cssopenwisp_controller/config/static/config/js/switcher.jsopenwisp_controller/config/tasks.pyopenwisp_controller/config/templates/admin/config/device/change_form.htmlopenwisp_controller/config/templates/admin/config/device_certificates_table.htmlopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pytests/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{md,rst}
⚙️ CodeRabbit configuration file
**/*.{md,rst}: Verify that documentation remains consistent with the implemented
behavior and does not reference deprecated or removed functionality.
Files:
docs/developer/extending.rstdocs/user/rest-api.rstdocs/user/intro.rstdocs/user/templates.rstdocs/index.rstdocs/user/settings.rstdocs/user/certificate-templates.rst
**/*
⚙️ 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:
docs/developer/extending.rstdocs/user/rest-api.rstopenwisp_controller/config/models.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/settings.pydocs/user/intro.rstdocs/user/templates.rstopenwisp_controller/config/static/config/css/admin.cssopenwisp_controller/config/templates/admin/config/device/change_form.htmldocs/index.rstopenwisp_controller/config/tasks.pyopenwisp_controller/config/base/vpn.pydocs/user/settings.rstdocs/user/certificate-templates.rstopenwisp_controller/config/templates/admin/config/device_certificates_table.htmlopenwisp_controller/config/handlers.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/static/config/js/switcher.jsopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.py
**/*.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/config/models.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.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/tests/test_config.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_vpn.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.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/config/models.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/template.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/config/models.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/template.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/config/models.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/handlers.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.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_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_device.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_config.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_vpn.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.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_config.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_vpn.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.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_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_device.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_config.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_device.py
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py
🪛 ast-grep (0.45.1)
openwisp_controller/config/tasks.py
[warning] 224-224: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/handlers.py
[warning] 81-81: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/base/device_certificate.py
[warning] 49-49: 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", "Template")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 232-232: 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/config/x509_admin.py
[warning] 9-9: 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", "Config")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 10-10: 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)
[warning] 11-11: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/base/config.py
[warning] 194-194: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/tests/test_selenium.py
[warning] 25-25: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_notifications", "Notification")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/config/tests/test_device.py
[warning] 32-32: 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("django_x509", "Cert")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 33-33: 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("django_x509", "Ca")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 34-34: 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", "OrganizationConfigSettings")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 994-994: use jsonify instead of json.dumps for JSON output
Context: json.dumps(expected_cert_ids, default=str)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 1207-1207: 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", "Template")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py
[info] 105-105: use help_text to document model columns
Context: models.CharField(blank=True, max_length=64, null=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/config/base/template.py
[warning] 42-42: 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("django_x509", "Cert")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 43-43: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 364-364: 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", "Config")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 429-429: 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", "DeviceCertificate")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 HTMLHint (1.9.2)
openwisp_controller/config/templates/admin/config/device_certificates_table.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (52)
docs/developer/extending.rst (1)
345-345: LGTM!docs/index.rst (1)
40-40: LGTM!docs/user/certificate-templates.rst (1)
1-283: LGTM!docs/user/intro.rst (1)
38-39: LGTM!docs/user/rest-api.rst (1)
1114-1115: LGTM!docs/user/settings.rst (1)
300-313: LGTM!Also applies to: 333-349, 362-378
docs/user/templates.rst (1)
212-231: LGTM!openwisp_controller/config/handlers.py (1)
46-53: LGTM!Also applies to: 56-93
openwisp_controller/config/base/device.py (1)
33-39: LGTM!Also applies to: 324-352, 354-380, 382-382, 391-391, 402-405
openwisp_controller/config/tests/test_admin.py (1)
13-13: LGTM!Also applies to: 39-42, 60-60, 1767-1802, 1804-1814, 1816-1855, 1857-1911, 2444-2445, 2508-2529
openwisp_controller/config/tests/test_api.py (1)
36-36: LGTM!Also applies to: 559-559, 747-747, 1340-1372, 1374-1389, 1391-1410, 1412-1435, 1437-1469, 1471-1504, 1506-1525, 1527-1563, 1565-1639
openwisp_controller/config/tests/test_config.py (1)
878-884: LGTM!openwisp_controller/config/tests/test_device.py (1)
1-11: LGTM!Also applies to: 32-34, 43-43, 748-845, 848-938, 940-1005, 1007-1089, 1092-1114, 1116-1192, 1194-1328
openwisp_controller/config/tests/test_selenium.py (1)
25-26: LGTM!Also applies to: 491-528, 530-561, 846-880
openwisp_controller/config/tests/test_template.py (1)
2-9: LGTM!Also applies to: 18-18, 32-32, 196-234, 633-633, 1015-1024, 1026-1048, 1050-1102, 1105-1244, 1246-1314, 1316-1447, 1449-1478, 1480-1563, 1588-1610, 1612-1662, 1664-1730
openwisp_controller/config/tests/test_vpn.py (1)
558-558: LGTM!Also applies to: 575-575
openwisp_controller/pki/tests/test_api.py (1)
155-155: LGTM!Also applies to: 247-270, 272-275, 288-308, 394-394, 405-405, 414-414
openwisp_controller/config/base/template.py (4)
42-51: LGTM!Also applies to: 76-98, 135-136
150-228: LGTM!
336-343: LGTM!Also applies to: 345-392, 394-447
507-520: LGTM!openwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.py (1)
22-152: LGTM!openwisp_controller/config/models.py (1)
5-5: LGTM!Also applies to: 97-104
tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py (1)
15-150: LGTM!openwisp_controller/config/admin.py (2)
53-53: LGTM!Also applies to: 66-66, 123-127
1152-1174: LGTM!Also applies to: 1221-1224
openwisp_controller/config/static/config/js/switcher.js (1)
4-15: LGTM!Also applies to: 42-55, 57-60
openwisp_controller/config/x509_admin.py (1)
43-60: LGTM!Also applies to: 63-126
openwisp_controller/config/templates/admin/config/device_certificates_table.html (1)
1-51: LGTM!Also applies to: 63-77
openwisp_controller/config/static/config/css/admin.css (2)
9-9: LGTM!Also applies to: 409-414, 419-433
415-418: 🎯 Functional CorrectnessConfirm the CSS custom properties in
openwisp-utils1.3. This repository does not define--ow-color-fg-ghost,--ow-color-fg-darker, or--ow-color-white. If the dependency does not define them, add fallbacks so the tooltip remains readable.openwisp_controller/config/templates/admin/config/device/change_form.html (1)
13-24: 🩺 Stability & AvailabilityNo issue:
i18nis loaded. The template header contains{% load admin_urls i18n l10n %}, so{% trans %}is available.> Likely an incorrect or invalid review comment.tests/openwisp2/sample_config/models.py (2)
99-105: Document the new swappable model indocs/developer/extending.rst.
DeviceCertificateis a new swappable model. The extending guide lists the models and settings that a derivative app must define. AddDeviceCertificateandCONFIG_DEVICECERTIFICATE_MODELthere.
5-5: LGTM!openwisp_controller/config/settings.py (1)
37-39: DocumentOPENWISP_CONTROLLER_REGENERATE_CERTS_ON_HARDWARE_CHANGE.This adds a public setting. Add it to
docs/user/settings.rst. Also update theOPENWISP_CONTROLLER_COMMON_NAME_FORMATentry, because standalone certificate templates now use that format throughgenerate_common_name.openwisp_controller/config/utils.py (2)
1-7: LGTM!Also applies to: 16-17, 110-112, 115-131, 134-144, 147-156
94-107: LGTM!openwisp_controller/config/base/vpn.py (2)
36-41: LGTM!Also applies to: 987-987, 1017-1017
1041-1042: 🗄️ Data Integrity & IntegrationConfirm the intended OU for VPN client certificates.
copy_ca_attributes(ca)now copiesorganizational_unit_nameinto each new VPN client certificate. Existing coverage checks OU copying for blueprint-based certificates, but no VPN-specific assertion exists. Confirm that consumers accept the CA OU in the certificate subject.tests/openwisp2/settings.py (1)
295-295: LGTM!openwisp_controller/config/base/device_certificate.py (4)
1-21: LGTM!Also applies to: 24-47
49-81: LGTM!Also applies to: 86-121, 126-134
136-141: LGTM!Also applies to: 143-156, 158-175, 177-190, 192-200, 202-207, 209-215, 217-223
225-243: LGTM!Also applies to: 249-296
openwisp_controller/config/base/config.py (3)
67-73: LGTM!Also applies to: 185-198
613-657: LGTM!
1087-1114: LGTM!Also applies to: 1142-1142
openwisp_controller/config/apps.py (1)
168-168: LGTM!Also applies to: 194-198, 220-224
openwisp_controller/config/tasks.py (1)
223-226: LGTM!openwisp_controller/config/api/serializers.py (3)
20-20: LGTM!Also applies to: 42-43, 53-62
79-94: LGTM!
343-361: LGTM!Also applies to: 363-375
| if ( | ||
| self.template.organization_id | ||
| and self.config.device.organization_id != self.template.organization_id | ||
| ): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the device lookups with self.config_id.
Lines 84 and 124 read self.config.device.organization_id. The enclosing conditions test only self.template_id and self.cert_id. Model.full_clean() calls clean() even when clean_fields() already reported the missing config. A DeviceCertificate that has a template or a certificate but no config therefore raises RelatedObjectDoesNotExist from clean() instead of returning a field error.
Add the config_id check to both conditions.
🐛 Proposed fix
if (
- self.template.organization_id
+ self.config_id
+ and self.template.organization_id
and self.config.device.organization_id != self.template.organization_id
): if (
- self.cert.organization_id
+ self.config_id
+ and self.cert.organization_id
and self.config.device.organization_id != self.cert.organization_id
):Also applies to: 122-125
🤖 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/base/device_certificate.py` around lines 82 - 85,
Update both conditions in the DeviceCertificate clean logic that access
self.config.device.organization_id to also require self.config_id, alongside the
existing template_id and cert_id guards, so clean() does not dereference a
missing config.
There was a problem hiding this comment.
This is correct and should be fixed. full_clean() on an incomplete instance currently raises RelatedObjectDoesNotExist while evaluating either organization check, instead of returning the expected validation errors. Please guard both dereferences with self.config_id and add an incomplete-instance regression test.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline openwisp_controller/config/base/device_certificate.py --match AbstractDeviceCertificate --view expanded
sed -n '45,140p' openwisp_controller/config/base/device_certificate.py
ast-grep outline openwisp_controller/config/tests/test_device.py --match 'TestDeviceCertificateModel|DeviceCertificate' --view expanded
rg -n -C 5 'full_clean\(\)|RelatedObjectDoesNotExist|organization_id|TestDeviceCertificateModel' \
openwisp_controller/config/tests/test_device.pyLength of output: 20800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'DeviceCertificate\(|ValidationError|assertRaises|assertRaisesMessage' \
openwisp_controller/config/tests/test_device.py
rg -n -C 4 'class .*Factory|def create_.*cert|Cert\.objects|Ca\.objects' \
openwisp_controller/config/tests/test_device.pyLength of output: 10734
@nemesifier, verification confirms the failure path.
AbstractDeviceCertificate.clean() accesses self.config.device.organization_id in both organization checks without verifying self.config_id. Existing tests in TestDeviceCertificateModel cover organization mismatches, but do not cover incomplete instances.
The change should:
- Add
self.config_idto both organization-check conditions. - Add regression cases for an incomplete instance with an organization-scoped certificate template and for one with an organization-scoped certificate.
- Assert that
full_clean()raisesValidationErrorwith the expected missingconfigfield error.
🐇 ✅
✏️ Learnings added
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/base/device_certificate.py:82-85
Timestamp: 2026-08-12T23:29:14.072Z
Learning: In `openwisp_controller/config/base/device_certificate.py`, `AbstractDeviceCertificate.clean()` must guard every access to `self.config.device` with `self.config_id`. Django `Model.full_clean()` invokes `clean()` even if `clean_fields()` found a missing required `config`, so an incomplete `DeviceCertificate` must raise `ValidationError` instead of `RelatedObjectDoesNotExist`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| dependencies = [ | ||
| ("config", "0065_whoisinfo_unreferenced_since"), | ||
| migrations.swappable_dependency(settings.DJANGO_X509_CA_MODEL), | ||
| migrations.swappable_dependency(settings.DJANGO_X509_CERT_MODEL), | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how earlier config migrations declare dependencies for swappable Config/Template models.
rg -n 'swappable_dependency\(' openwisp_controller/config/migrations | head -40
rg -n -C3 'CONFIG_CONFIG_MODEL|CONFIG_TEMPLATE_MODEL' openwisp_controller/config/migrations | head -60Repository: openwisp/openwisp-controller
Length of output: 5741
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration 0066 ---'
sed -n '1,180p' openwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.py
printf '%s\n' '--- migration 0001 and model declarations ---'
rg -n -C4 'class (Config|Template)|swappable|CONFIG_CONFIG_MODEL|CONFIG_TEMPLATE_MODEL' openwisp_controller/config openwisp_controller --glob '*.py' | head -160
printf '%s\n' '--- migration dependencies around config models ---'
for f in openwisp_controller/config/migrations/0043_devicegroup_templates.py openwisp_controller/config/migrations/0056_vpnclient_template.py openwisp_controller/config/migrations/0058_alter_vpnclient_template.py; do
echo "### $f"
sed -n '1,45p' "$f"
doneRepository: openwisp/openwisp-controller
Length of output: 19860
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact model and setting definitions ---'
rg -n -C6 'swappable_setting\("config"|CONFIG_CONFIG_MODEL|CONFIG_TEMPLATE_MODEL|class (Config|Template)\b' . --glob '*.py' --glob '*.toml' --glob '*.ini' --glob '*.cfg' --glob '*.yaml' --glob '*.yml' | head -240
printf '%s\n' '--- config migration dependency graph ---'
python3 - <<'PY'
from pathlib import Path
import ast
root = Path("openwisp_controller/config/migrations")
for path in sorted(root.glob("*.py")):
tree = ast.parse(path.read_text())
deps = None
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name == "Migration":
for item in node.body:
if isinstance(item, ast.Assign) and any(
isinstance(t, ast.Name) and t.id == "dependencies" for t in item.targets
):
deps = ast.unparse(item.value)
if deps and ("CONFIG_" in deps or path.name.startswith(("0043", "0056", "0066"))):
print(path.name, "=>", deps)
PY
printf '%s\n' '--- migration files that define config/template models ---'
rg -l 'CreateModel\(\s*$' openwisp_controller/config/migrations --glob '*.py' | head -20
rg -n -C3 'name="(config|template)"|name="Config"|name="Template"' openwisp_controller/config/migrations/000*.py | head -160Repository: openwisp/openwisp-controller
Length of output: 27426
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration files for swapped test app ---'
git ls-files tests/openwisp2/sample_config/migrations tests/openwisp2/sample_config | head -80
printf '%s\n' '--- initial config migration dependencies and swappable metadata ---'
sed -n '1,75p' openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py
rg -n -C4 '"swappable"|CONFIG_CONFIG_MODEL|CONFIG_TEMPLATE_MODEL' openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py
printf '%s\n' '--- sample config migration declarations ---'
if [ -d tests/openwisp2/sample_config/migrations ]; then
for f in tests/openwisp2/sample_config/migrations/*.py; do
echo "### $f"
sed -n '1,42p' "$f"
done
fi
printf '%s\n' '--- Django availability and swappable_dependency implementation ---'
python3 - <<'PY'
try:
import django
from django.db.migrations import swappable_dependency
import inspect
print("django", django.get_version())
print(inspect.getsource(swappable_dependency))
except Exception as exc:
print(type(exc).__name__, str(exc))
PYRepository: openwisp/openwisp-controller
Length of output: 4055
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- swapped-app migration 0012 ---'
sed -n '1,42p' tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py
printf '%s\n' '--- swapped-app migration 0001 dependencies ---'
sed -n '1,32p' tests/openwisp2/sample_config/migrations/0001_initial.py
printf '%s\n' '--- Django swappable_dependency implementation ---'
python3 - <<'PY'
try:
import django
from django.db.migrations import swappable_dependency
import inspect
print("django", django.get_version())
print(inspect.getsource(swappable_dependency))
except Exception as exc:
print(type(exc).__name__, str(exc))
PY
printf '%s\n' '--- compare dependency blocks ---'
python3 - <<'PY'
from pathlib import Path
for name in (
"openwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.py",
"tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py",
):
text = Path(name).read_text()
start = text.index(" dependencies = [")
end = text.index(" operations = [", start)
print(name)
print(text[start:end], end="")
PYRepository: openwisp/openwisp-controller
Length of output: 3256
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- full swapped-app migration 0012 ---'
sed -n '1,190p' tests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.py
printf '%s\n' '--- all sample_config migration dependencies ---'
python3 - <<'PY'
from pathlib import Path
import ast
for path in sorted(Path("tests/openwisp2/sample_config/migrations").glob("*.py")):
try:
tree = ast.parse(path.read_text())
except SyntaxError:
continue
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name == "Migration":
for item in node.body:
if isinstance(item, ast.Assign) and any(
isinstance(t, ast.Name) and t.id == "dependencies" for t in item.targets
):
print(path.name, "=>", ast.unparse(item.value))
PY
printf '%s\n' '--- swapper dependency usage in migrations ---'
rg -n -C3 'swapper\.(dependency|split)|dependency\(\*split|swappable_dependency' . --glob '*/migrations/*.py' --glob '*.py' | head -220
printf '%s\n' '--- test settings for all CONFIG model swaps ---'
sed -n '270,305p' tests/openwisp2/settings.pyRepository: openwisp/openwisp-controller
Length of output: 24998
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migration execution helpers and tests ---'
sed -n '1,180p' openwisp_controller/migrations.py
rg -n -C5 'migrat|MigrationExecutor|sample_config|CONFIG_TEMPLATE_MODEL|CONFIG_CONFIG_MODEL' tests --glob '*.py' | head -260
printf '%s\n' '--- model Meta swappability and abstract relations ---'
sed -n '1,155p' openwisp_controller/config/models.py
rg -n -C8 'class Abstract(DeviceCertificate|Config|Template)|ForeignKey\(.*config|ForeignKey\(.*template' openwisp_controller/config/base openwisp_controller/config --glob '*.py' | head -220Repository: openwisp/openwisp-controller
Length of output: 41258
Add swappable dependencies for Config and Template.
Add migrations.swappable_dependency(settings.CONFIG_CONFIG_MODEL) and migrations.swappable_dependency(settings.CONFIG_TEMPLATE_MODEL). Without them, 0066_template_blueprint_cert_template_ca_and_more can run before the swapped model tables exist.
🤖 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/migrations/0066_template_blueprint_cert_template_ca_and_more.py`
around lines 16 - 20, Add swappable dependencies for
settings.CONFIG_CONFIG_MODEL and settings.CONFIG_TEMPLATE_MODEL to the
dependencies list in migration
0066_template_blueprint_cert_template_ca_and_more, alongside the existing Django
X.509 dependencies, so the migration waits for the swapped Config and Template
model tables.
There was a problem hiding this comment.
The concern is valid if Config or Template can be swapped independently while DeviceCertificate remains in the default app. I have not included it among the immediate blockers because that support boundary needs maintainer confirmation.
There was a problem hiding this comment.
@nemesifier, agreed. The dependency is required only if independent swapping of Config or Template is supported while DeviceCertificate remains in the default config app.
I will keep this thread open pending confirmation of that support boundary. If that configuration is supported, 0066_template_blueprint_cert_template_ca_and_more must add the two migrations.swappable_dependency(...) entries.
✏️ Learnings added
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.py:16-20
Timestamp: 2026-08-12T23:28:59.273Z
Learning: For openwisp/openwisp-controller migration `openwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.py`, dependencies on `settings.CONFIG_CONFIG_MODEL` and `settings.CONFIG_TEMPLATE_MODEL` are required only if `Config` or `Template` can be swapped independently while `DeviceCertificate` remains in the default `config` app. This support boundary requires maintainer confirmation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
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.
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.
You are interacting with an AI system.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I found six correctness issues that need to be addressed before merging. The inline comments contain the reproductions and smallest fixes. I also reviewed the latest CodeRabbit findings directly in their threads; three are valid correctness issues, while the remaining suggestions are invalid, cosmetic, or non-blocking.
| certificate or a certificate not linked to a VpnClient or | ||
| DeviceCertificate does not affect any configuration. | ||
| """ | ||
| if cert.revoked: |
There was a problem hiding this comment.
I can revoke an assigned standalone certificate from the PKI admin/API and the configuration remains applied while get_cert_context() continues exposing the revoked certificate and its private key. This early return prevents checksum invalidation, and the context builder does not exclude revoked certificates, so the device can keep downloading credentials that the operator explicitly revoked. Please remove revoked standalone certificates from get_cert_context() and still resolve their DeviceCertificate config here so revocation marks it as modified. Add a regression test covering both outcomes.
| ] | ||
| readonly_fields = ["system_context"] | ||
| autocomplete_fields = ["vpn"] | ||
| autocomplete_fields = ["vpn", "ca", "blueprint_cert"] |
There was a problem hiding this comment.
These autocomplete fields prevent organization administrators from selecting shared CAs and shared blueprint certificates. I reproduced this through the admin autocomplete endpoint: organization-owned objects are returned, but equivalent organization=None objects are omitted because the lookup uses CaAdmin.get_queryset() or CertAdmin.get_queryset(), not TemplateAdmin.multitenant_shared_relations. The model explicitly allows shared relations. Please make these two lookups source-aware so shared objects are available from TemplateAdmin without exposing them in the normal PKI changelist, and cover both fields with an organization-administrator test.
|
|
||
|
|
||
| class AbstractDeviceCertificate(TimeStampedEditableModel): | ||
| config = models.ForeignKey( |
There was a problem hiding this comment.
This model is swappable, but its reverse relations are not stable. The implementation hardcodes devicecertificate_set in base/config.py and base/template.py, and devicecertificate__ in x509_admin.py. A valid custom model with a different class name gets different default reverse names, breaking certificate creation and cleanup, context generation, status updates, and the PKI device filter. Please define stable related_name and related_query_name values on this relation and use them consistently. The sample model keeps the same class name, so add coverage with a swapped concrete model whose name differs.
| ) | ||
| } | ||
| ) | ||
| if ( |
There was a problem hiding this comment.
This organization invariant is checked only when assigning the DeviceCertificate. I can later change the bound Cert.organization to another organization and save it successfully. The original organization's config still renders that certificate and private key, while the PKI object is now visible to administrators of the new organization. Please reject organization changes while a certificate is referenced by a DeviceCertificate, using a swapped-model-aware validation or pre-save guard, and add a cross-organization regression test.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I left inline comments on the remaining issues.
| update_fields = kwargs.get("update_fields") | ||
| is_positional = False | ||
| if update_fields is None and len(args) > 3: | ||
| update_fields = args[3] |
There was a problem hiding this comment.
This code is quite obscure to me? Can you add a preceding comment reminding us why it's needed?
| 2, | ||
| "Stale DeviceCertificate row should still exist", | ||
| ) | ||
|
|
There was a problem hiding this comment.
avoid needless blank lines please
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ |
There was a problem hiding this comment.
Please add migrations.swappable_dependency(settings.CONFIG_CONFIG_MODEL) and migrations.swappable_dependency(settings.CONFIG_TEMPLATE_MODEL). They are redundant but harmless for the default same-app models, and make migration ordering correct when Config or Template is swapped.
| from .x509_admin import get_device_certificate_details, register_cert_admin_filter | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| prefix = "config/" |
There was a problem hiding this comment.
I do not see a requirement to show CA and Blueprint Certificate in the template changelist. Issue #1357 requires these fields in the form, not in list_display. Please remove these two columns and avoid extra width and per-row queries. If there is a concrete reason to keep them, add list_select_related.
| Config = load_model("config", "Config") | ||
| Organization = load_model("openwisp_users", "Organization") | ||
| DeviceCertificate = load_model("config", "DeviceCertificate") | ||
|
|
There was a problem hiding this comment.
Issue #1361 requires these fields in the existing template API, so they must stay. Please update the existing list and detail query-count tests to use certificate templates with populated ca and blueprint_cert, assert both returned IDs, and retain the query counts. This proves the new fields do not introduce related-object queries.
| config={"system": {"hostname": "test_router"}}, | ||
| ) | ||
| device = self._create_device(organization=org) | ||
| config = self._create_config(device=device) |
There was a problem hiding this comment.
This does not exercise the NULL case described by the test name and docstring because it asserts cert_id is non-NULL. Please create a pending relation by setting cert=None with QuerySet.update(), refresh it, and assert the NULL precondition. Keep a separate assigned certificate and verify it remains excluded while an unassigned blueprint remains available. Please also replace the docstring with Ensure pending device certificates do not hide available blueprints.
…s/1356-extend-abstract-template
Addressed latest reviews and added new migration to fix the CI
Removed stale comment and added explanation.
Updated documenetation after changes.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
See my comments below, let's discuss the problems caused by changing organization at the earliest chance
| :guilabel:`Automatic certificate provisioning` (``auto_cert``) | ||
| Certificate generator templates always use automatic certificate | ||
| provisioning. An X.509 certificate is automatically created and signed | ||
| by the template's CA the moment the template is assigned to a device | ||
| configuration. |
There was a problem hiding this comment.
(P3) This is presented as a form field, but the admin hides auto_cert for certificate templates and the API does not expose it; I think we decided to remove / hide this, right? Is this documentation section outadated or am I missing something?
| To prevent breaking the cryptographic binding with devices that are | ||
| already using a template, certain destructive changes are blocked while | ||
| the template is assigned to *active* or *activating* device | ||
| configurations. |
There was a problem hiding this comment.
There is no activating Config status. The implementation blocks assignments in every status except deactivating and deactivated, so this wording does not describe the actual lock. Please state that exact condition or simply say configurations that are not deactivating or deactivated. (P3)
| f"{prefix}_pem": dc.cert.certificate, | ||
| f"{prefix}_key_path": key_path, | ||
| f"{prefix}_key": dc.cert.private_key, | ||
| f"{prefix}_uuid": str(dc.cert.id), |
There was a problem hiding this comment.
There's something wrong here. The var name suffix indicates that this should be a UUID, but if I am not mistaken Cert.id is an autoincrement integer field. So there's a discrepancy in expectations here.
| self.config_id | ||
| and self.cert.organization_id | ||
| and self.config.device.organization_id != self.cert.organization_id | ||
| ): | ||
| raise ValidationError( | ||
| { | ||
| "cert": _( | ||
| "The organization of the certificate must match " | ||
| "the organization of the device." |
There was a problem hiding this comment.
This validation is executed only on creation. If a device is moved to a different organization, it won't be executed. Did we already discuss this? I don't see an easy solution right now.
I think we'll need to make these changes to solve it for good:
| attrs = copy_ca_attributes(ca, blueprint) | ||
| extensions = get_client_extensions( | ||
| blueprint, hardware_oids=self._get_hardware_oid_extensions() | ||
| ) |
There was a problem hiding this comment.
P2 A valid blueprint may already contain either reserved OpenWISP hardware OID. The failure path is:
- Create a valid blueprint certificate containing
1.3.6.1.4.1.65901.1or1.3.6.1.4.1.65901.2. - Create a certificate template with that blueprint.
- Assign the template to a device configuration.
get_client_extensions()copies the blueprint extensions and appends the same hardware OIDs for the device.cert.full_clean()rejects the duplicate OID, so template assignment fails.
Please replace inherited values for these two OIDs, or reject them during template validation, and add a regression test.
| _changed_checked_fields = [ | ||
| "ca_id", | ||
| "blueprint_cert_id", | ||
| "type", | ||
| ] |
There was a problem hiding this comment.
P1 The active-template lock does not track organization_id. With a shared CA and blueprint, an assigned certificate template can be moved from organization A to B while it remains linked to A's configs; B's admins then own a template whose changes affect A's devices. Please include organization changes in the protected snapshot and reject them while active configs reference the certificate template, with a cross-organization regression test.
This is related to, but distinct from, #1459. #1459 covers moving a device between organizations and reconciling its assigned templates. This path moves an active template itself: organization B gains control of a template still deployed on organization A devices. Device-move reconciliation does not handle that transfer.
| def refresh_from_db(self, *args, **kwargs): | ||
| super().refresh_from_db(*args, **kwargs) | ||
| self._set_initial_values_for_changed_checked_fields() |
There was a problem hiding this comment.
P2 A partial refresh resets every protected snapshot, including fields that were not refreshed. For example, after setting ca to a new value, refresh_from_db(fields=["name"]) leaves the dirty ca in memory but records it as the initial ca, so full_clean() no longer detects the active-template mutation.
No production caller currently partially refreshes a Template, but this is an actual stale-snapshot bug and is inexpensive to fix. Capture fields in refresh_from_db() and pass it to _set_initial_values_for_changed_checked_fields(update_fields=fields), handling the positional fields argument too. The existing mutation tests do not exercise refresh behavior, so add one focused test beside test_validate_cert_template_changes_same_instance_after_save rather than distorting an unrelated test.
| email=ca.email, | ||
| common_name=common_name, | ||
| extensions=server_extensions, | ||
| extensions=list(DEFAULT_CLIENT_EXTENSIONS), |
There was a problem hiding this comment.
P3 This refactor changed a fresh per-certificate dictionary into a shallow copy of a module-level list, so the nested extension dictionary is shared by every generated VPN certificate. Please preserve the previous isolation.
The existing helper is get_client_extensions() in config.utils. With no arguments it returns a deep copy of DEFAULT_CLIENT_EXTENSIONS, exactly what this call needs. Import and call get_client_extensions() here instead of list(DEFAULT_CLIENT_EXTENSIONS); this avoids a new direct deepcopy and keeps both certificate-provisioning paths consistent.
| def test_template_create_api_org_scoping(self): | ||
| """Rejects CA or Blueprint from a different organization""" | ||
| org1 = self._get_org() | ||
| org2 = self._create_org(name="Org2", slug="org2") | ||
| ca_org2 = self._create_ca(name="CA2", common_name="CA2", organization=org2) | ||
| path = reverse("config_api:template_list") | ||
| data = self._template_data | ||
| data.update( | ||
| { | ||
| "name": "Org Scope Template", | ||
| "type": "cert", | ||
| "ca": ca_org2.pk, | ||
| "organization": str(org1.pk), | ||
| "config": {}, | ||
| } | ||
| ) | ||
| r = self.client.post(path, data, content_type="application/json") | ||
| self.assertEqual(r.status_code, 400) | ||
| self.assertIn("organization", r.data) | ||
| self.assertIn("related CA match", str(r.data["organization"])) |
There was a problem hiding this comment.
P3 This test does not verify API access control because it runs as the default superuser, who is allowed to access every organization.
Use a normal manager of organization A and try to create a certificate template for organization A with these values:
- A CA owned by organization B.
- A blueprint certificate owned by organization B.
- A shared CA and a shared blueprint certificate.
The first two requests must return 400. The shared values must be accepted. This proves that the API filters both related fields by the requesting manager's organizations, as required by #1361.
| def save(self, *args, **kwargs): | ||
| update_fields = kwargs.get("update_fields") | ||
| if update_fields is None and len(args) > 3: | ||
| update_fields = args[3] | ||
| super().save(*args, **kwargs) | ||
| if update_fields is None or {"organization", "organization_id"}.intersection( | ||
| update_fields | ||
| ): | ||
| self._initial_organization_id = self.organization_id |
There was a problem hiding this comment.
P2 The bound-certificate organization check runs from pre_save, after serializer and ModelForm validation. Updating the organization of a bound certificate therefore passes validation and raises an uncaught Django ValidationError here, yielding an API or admin 500 instead of a field error.
Failing regression to add in openwisp_controller/pki/tests/test_api.py:
- Create a device configuration with an assigned certificate template, then retrieve its generated certificate.
- Create a second organization.
- PATCH
reverse("pki_api:cert_detail", args=[cert.pk])with{"organization": str(org2.pk)}. - Assert a 400 response with an organization error and confirm the certificate still belongs to the first organization.
The current code reaches the pre_save receiver and returns a 500. Add the corresponding admin form regression after moving the invariant into validation, while retaining the pre_save guard for direct ORM callers.
Addressed all latest review with 1 pending to address
Fixed 500 status code issue and added tests
4 participants
Checklist
Reference to Existing Issue
Closes #1356
Closes #1377
Closes #1357
Closes #1361
Closes #1358
Closes #1360
Closes #1359
Description of Changes
This PR establishes the database architecture, UI, API and lifecycle for standalone X.509 certificate templates.
Manual test plan
Setup
PKI->Certification Authoritiesand create two CAs:CA-1andCA-2.PKI->Certificatesand create two certificates to act as blueprints:Blueprint-1(Must useCA-1)Blueprint-2(Must useCA-2)Devicesand create a device (test-device).Template Creation and Validation
Configuration->Templatesand clickADD TEMPLATE.Certificate.CA-1.Blueprint-2(which belongs toCA-2). Try to save.Blueprint-1. Name the templateActive-Cert-Template. Save it.Device Provisioning
test-device.Active-Cert-Template. Save.PKI->Certificates.test-device. Its status should be valid (not revoked).Active Mutation Locks
Configuration->Templatesand editActive-Cert-Template(which is now assigned to an active device).Generic. Try to save.CA-2. Try to save.Blueprint-2(ensure you also change the CA so they match, triggering the active lock). Try to save.Revocation on Removal
test-device.Active-Cert-Templateentirely from the templates list. Save.PKI->Certificatesand locate the device's certificate.Context Configuration Injection
Go to
Configuration->Templates, openActive-Cert-Template, and copy its UUID from the URL bar (removing the dashes so it is a 32-character hex string).In the JSON configuration editor for the template, add a configuration block that references the certificate's UUID variables:
(Note: Replace
<uuid>with the actual 32-character hex string of the template).Click Save.
Go back to the Configuration page for
test-device(which has this template assigned) and click thePreview configurationbutton.Expected Result: The variables should be successfully resolved. In the preview, you should see the generated path (e.g.,
/etc/x509/cert-<uuid>.pem) and the literal-----BEGIN CERTIFICATE-----text instead of the raw{{ }}template tags.output.mp4