[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 reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Review Summaries (19 snapshots, latest commit 9b17fcd)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9b17fcd)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous review (commit a8a0d36)Status: No Issues Found | Recommendation: Merge Files Reviewed (13 files)
All previous review findings have been addressed in this incremental diff. Previous review (commit 5cbd01c)Status: No Issues Found | Recommendation: Merge Files Reviewed (13 files)
All previous review findings have been addressed in this incremental diff. Previous review (commit 83134fc)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous review (commit c62c583)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous review (commit 5fc22c3)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous review (commit c09b330)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous review (commit 1df332c)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 04bfa80)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit db8c592)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files - incremental since previous review)
NotesThe incremental changes refine the device certificate admin presentation and improve error messaging specificity:
No critical bugs or security vulnerabilities found in the changed code. Previous review (commit 6988562)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files - incremental since previous review)
NotesThe incremental changes refine the device certificate admin presentation and improve error messaging specificity:
No critical bugs or security vulnerabilities found in the changed code. Previous review (commit c803d75)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files - incremental since previous review)
NotesThe incremental changes are purely migration reorganization:
Migration sequence integrity verified ( Previous review (commit bf08a9c)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 0a9c7d1)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (18 files in incremental diff)
Notes: The refactor moving cert-template validation from Previous review (commit dda2865)Status: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous review (commit 1d117a2)Status: No Issues Found | Recommendation: Merge Incremental Review (since 375844d)This delta is a test-only refactor in Files Reviewed (incremental — 1 file)
Previous review (commit 375844d)Status: No Issues Found | Recommendation: Merge Incremental Review (since c12aee9)This delta refactors the hardware-drift detection (name/MAC change → certificate regeneration) to live entirely in the model rather than a separate
Ordering is correct: Tests cover the refactored paths: name change, MAC change, unrelated-field save (ignored), Files Reviewed (incremental — 2 files)
Previous review (commit c12aee9)Status: No Issues Found | Recommendation: Merge Incremental Review (since b9a9d2a)Since the last review, the branch merged the base branch ( The author's own changes in this delta are clean and well-tested:
New/updated tests cover the notes field (admin ordering, search, API create/list/patch), the cert-template type-mutation lock, the certificate-details admin rendering, and the WHOIS Selenium error helper. No critical bugs or security vulnerabilities in the changed code. Files Reviewed (incremental — feature scope)
Previous review (commit b9a9d2a)Status: No Issues Found | Recommendation: Merge Incremental Review (since 52c7544)The new commit (
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by step-3.7-flash · Input: 143.7K · Output: 27.7K · Cached: 3.6M |
|
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
c09b330 to
5fc22c3
Compare
nemesifier
moved this from In progress
to In review
in [GSoC26] X.509 Certificate Generator Templates
Addressed review comments involving removal of auto_cert from DeviceCertificate and blocking cert deletion from active cert
…s/1356-extend-abstract-template
Fixed migrations causing CI to fail after rebasing
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/api/serializers.py (1)
342-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unnecessary blank lines and document the non-obvious certificate cleanup.
Two issues in
update():
- Line 371 and line 374 are blank lines inside the method body. The path instructions for
**/*.pyrequire avoiding unnecessary blank lines inside function and method bodies.- Line 362 deletes all
DeviceCertificaterows forinstance.configimmediately after the organization is reassigned, but beforetemplates.clear(). This ordering is non-obvious and was the subject of extensive back-and-forth in prior reviews (stale certificates on organization change). The existing comment at lines 355-360 explains whyorganizationis set early, but not why the certificate deletion must happen at this exact point. Add a short comment explaining that certificates tied to the old organization's templates must be revoked/removed before required templates for the new organization are re-added.♻️ Proposed fix
instance.config.device.organization = validated_data.get( "organization" ) + # Delete existing device certificates before clearing templates + # so certificates bound to the old organization's templates are + # cleaned up and not left orphaned when required templates for + # the new organization are re-added below. DeviceCertificate.objects.filter(config=instance.config).delete() instance.config.templates.clear() Config.enforce_required_templates( action="post_clear", instance=instance.config, sender=instance.config.templates, pk_set=None, raw_data=raw_data_for_signal_handlers, ) - if config_data: self._update_config(instance, config_data) - return super().update(instance, validated_data)As per path instructions: "Avoid unnecessary blank lines inside function and method bodies" and "Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it."
🤖 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/api/serializers.py` around lines 342 - 375, In update(), remove the unnecessary blank lines within the method body. Before DeviceCertificate.objects.filter(config=instance.config).delete(), add a concise comment explaining that certificates associated with the old organization’s templates must be removed before templates are cleared and required templates for the new organization are re-added; preserve the existing operation order.Source: Path instructions
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_template.py (1)
1525-1546: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe NULL
cert_idregression is no longer covered.The docstring states that a
DeviceCertificatewithcert=Nonemust not poisonget_unassigned_certs(). The fixture provisions the row throughconfig.templates.add(template), soauto_certalways setscert, and line 1542 assertscert_idis notNone. Thecert_id__isnull=Falsefilter inget_unassigned_certs()stays untested. This repeats a finding that was previously confirmed as fixed.A second
DeviceCertificate.objects.create(...)for the sameconfigandtemplatewould violate the uniqueness constraint. Force the NULL state with a querysetupdate()instead.🐛 Proposed fix
config.templates.add(template) device_cert = DeviceCertificate.objects.get(config=config, template=template) - self.assertIsNotNone(device_cert.cert_id) + # Bypass save() so that auto_cert cannot re-provision the certificate. + 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()🤖 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 1525 - 1546, Update test_get_unassigned_certs_with_null_device_cert so the existing DeviceCertificate is forced into the NULL state using a queryset update on its cert field, rather than asserting cert_id is non-NULL. Keep the same config/template fixture and then verify get_unassigned_certs() still includes unassigned_cert, covering the cert_id__isnull=False filtering without creating a duplicate row.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/templates.rst`:
- Around line 226-228: The certificate template documentation at
docs/user/templates.rst lines 226-228 must state that automatic X.509
provisioning and revocation occur only when auto_cert is enabled, distinguishing
this lifecycle from manually assigned certificates. Update the hardware-change
regeneration text at docs/user/settings.rst lines 333-347 to limit it to active,
automatically managed certificates rather than all existing X.509 certificates.
In `@openwisp_controller/config/base/config.py`:
- Around line 1113-1115: The rendering path in
openwisp_controller/config/base/config.py:1113-1115 must use
devicecertificate_set.all() so Django can consume the prefetch cache; update the
related model configuration in
openwisp_controller/config/base/template.py:243-252 to keep
select_related("template", "cert") and order_by("created") exclusively in the
Prefetch queryset, and move the default ordering to
DeviceCertificate.Meta.ordering for deterministic non-prefetched results.
- Around line 602-618: The stale certificate_updated handler should no longer
perform duplicate certificate-dependency resolution. Remove
ConfigConfig.certificate_updated and any references to it, or disconnect its
signal registration if one exists, while preserving the active
django_x509.Cert.post_save registration through
ConfigConfig.get_cache_dependencies().
In `@openwisp_controller/config/base/device_certificate.py`:
- Around line 250-254: Normalize both sides of the expected certificate ID
comparison in the active-device-certificate loop: convert keys and expected
values from expected_cert_ids, as well as dc.id and dc.cert_id, to a consistent
string representation before building or querying expected_map. Preserve the
existing mismatch continue behavior so serialized list-of-string pairs and
direct UUID inputs both enforce the idempotency guard.
- Around line 247-249: Materialize active_device_certs once after applying
select_related, then check whether the materialized collection is empty before
returning and iterate over that same collection. Update the surrounding
certificate-processing flow without re-evaluating the locked queryset or issuing
a separate exists() query.
In `@openwisp_controller/config/base/template.py`:
- Around line 168-174: Update _set_initial_values_for_changed_checked_fields and
its call from save so tracked initial values refresh only for fields included in
update_fields, while preserving the current behavior when all fields are saved.
Support update_fields supplied either as a keyword or positional save argument,
and leave refresh_from_db unchanged.
- Around line 435-443: Update the certificate-template validation around the
type and auto_cert handling to normalize auto_cert to true when self.type ==
"cert" instead of rejecting creation. Preserve existing behavior for
non-certificate templates and ensure REST API creation succeeds even when
DEFAULT_AUTO_CERT is false.
In `@openwisp_controller/config/tests/test_api.py`:
- Around line 1340-1358: Add a subtest to test_template_create_cert_type_api
that patches DEFAULT_AUTO_CERT to False, posts the same certificate template
payload without auto_cert, and asserts the expected API response for
AbstractTemplate.clean() rejecting it. Keep the existing successful
default-setting assertion unchanged and use the project’s established
settings-patching mechanism.
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 1082-1093: Update test_str_pending_generation to create a
DeviceCertificate row without a certificate and assert its string representation
contains “Pending Generation”; retain the existing populated-certificate
assertion only if testing both branches, or rename the test to reflect the
non-pending behavior.
In `@openwisp_controller/config/tests/test_selenium.py`:
- Around line 530-561: Rename test_device_property_change_notification to
reflect certificate regeneration after device identity fields change, while
preserving its existing test flow and assertion. Inspect the notification
message source and widget styles to confirm the rendered text remains exactly
“Device identity fields changed” with no CSS text transformation; adjust only
the relevant source or styling if necessary.
---
Outside diff comments:
In `@openwisp_controller/config/api/serializers.py`:
- Around line 342-375: In update(), remove the unnecessary blank lines within
the method body. Before
DeviceCertificate.objects.filter(config=instance.config).delete(), add a concise
comment explaining that certificates associated with the old organization’s
templates must be removed before templates are cleared and required templates
for the new organization are re-added; preserve the existing operation order.
---
Duplicate comments:
In `@openwisp_controller/config/tests/test_template.py`:
- Around line 1525-1546: Update test_get_unassigned_certs_with_null_device_cert
so the existing DeviceCertificate is forced into the NULL state using a queryset
update on its cert field, rather than asserting cert_id is non-NULL. Keep the
same config/template fixture and then verify get_unassigned_certs() still
includes unassigned_cert, covering the cert_id__isnull=False filtering without
creating a duplicate row.
🪄 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 Plus
Run ID: 31460d85-cdd5-4ae6-b00d-dd6e13d39f5f
⛔ 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 (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/tests/test_vpn.pyopenwisp_controller/config/apps.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/models.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_template.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/tests/test_vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/models.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/device.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/tests/test_vpn.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/models.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/device.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/tests/test_vpn.pyopenwisp_controller/config/apps.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/models.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/x509_admin.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/utils.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_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_vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_template.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_vpn.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_template.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_vpn.pytests/openwisp2/sample_config/migrations/0012_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_template.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_vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_template.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_vpn.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_device.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/0012_template_blueprint_cert_template_ca_and_more.py
🪛 ast-grep (0.45.0)
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/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/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/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/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/device_certificate.py
[warning] 48-48: 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] 231-231: 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/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)
[warning] 603-603: 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] 31-31: 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] 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", "Ca")
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("config", "OrganizationConfigSettings")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 1186-1186: 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)
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] 322-322: 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] 387-387: 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/user/certificate-templates.rst (6)
35-37: Verify the certificate-template screenshot asset.The image and target use an external raw GitHub path. The asset is not included in the supplied changes. Confirm that the file exists before publishing the documentation.
#!/bin/bash set -euo pipefail curl -fsSIL \ "https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/1.4/certificate-templates/certificate-template.png"
229-240: Verify the generated API documentation.This section documents certificate-template API fields, but the supplied files do not verify the live OpenAPI schema or the DRF browsable API. Confirm that the intended
certfields, organization filtering, RBAC, and validation appear in both interfaces.#!/bin/bash set -euo pipefail rg -n -C 4 \ 'blueprint_cert|auto_cert|TemplateSerializer|type.*cert' \ openwisp_controller/config/api/serializers.py \ openwisp_controller/config/tests/test_api.py \ openwisp_controller/config/tests/test_selenium.py
1-33: LGTM!
39-116: LGTM!
118-226: LGTM!
242-283: LGTM!docs/developer/extending.rst (1)
345-345: LGTM!docs/index.rst (1)
40-40: LGTM!docs/user/intro.rst (1)
38-39: LGTM!docs/user/rest-api.rst (1)
1114-1115: LGTM!docs/user/settings.rst (2)
311-313: LGTM!
360-361: LGTM!Also applies to: 370-376
openwisp_controller/config/api/serializers.py (2)
20-20: LGTM!Also applies to: 42-43, 53-62
79-94: 🗄️ Data Integrity & IntegrationConfirm model-level validation still covers what
validate()no longer checks.
validate()now only clearscaandblueprint_certfor non-certtemplates. It no longer performs the CA-required check, the blueprint/CA compatibility check, or the active-template mutation lock check that earlier revisions implemented directly in the serializer. This matches the direction requested earlier (delegate toAbstractTemplate.clean()/_validate_cert_template_changes()), buttemplate.pyis not part of this review batch.Confirm the model raises
ValidationErrorwith the same field keys (ca,blueprint_cert,type) soValidatedModelSerializersurfaces them under the same DRF error keys the test suite expects (test_template_create_cert_rejects_without_ca,test_template_create_api_blueprint_ca_mismatch,test_template_update_api_active_change_blocked).#!/bin/bash # Confirm the model still raises field-keyed ValidationErrors for cert-template invariants rg -n "_clean_cert_template|_validate_cert_template_changes|def clean" -A 25 openwisp_controller/config/base/template.pyAs per coding guidelines: "Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data" and "Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code."
openwisp_controller/config/admin.py (2)
52-52: LGTM!Also applies to: 65-65, 122-126, 1073-1074, 1088-1097, 1110-1110
929-932: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing
_has_config()helper instead of duplicating the check.
Device._has_config()(hasattr(self, "config")) already exists and is used elsewhere in this same file (line 154, line 981). This new code reimplements the same check inline with a redundant extra condition:hasattr(device, "config")already impliesdevice.configis a valid, truthy object, soand device.configadds nothing.♻️ Proposed fix
- if hasattr(device, "config") and device.config: + if device._has_config(): ctx["certificate_details"] = get_device_certificate_details( device.config )> Likely an incorrect or invalid review comment.openwisp_controller/config/x509_admin.py (1)
1-127: LGTM!openwisp_controller/config/static/config/js/switcher.js (1)
4-60: 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 (1)
1-78: LGTM!openwisp_controller/config/static/config/css/admin.css (1)
9-9: LGTM!Also applies to: 409-458
openwisp_controller/config/base/template.py (6)
9-9: LGTM!Also applies to: 25-29, 40-49
74-96: LGTM!Also applies to: 133-134
294-301: LGTM!
303-350: LGTM!
352-406: LGTM!
176-185: 🩺 Stability & AvailabilityNo change needed for
only()FK attnames.
only("ca_id")andonly("blueprint_cert_id")are valid deferred names for theseForeignKeycolumns, so the fallback query does not need to map to field names.> Likely an incorrect or invalid review comment.openwisp_controller/config/migrations/0066_template_blueprint_cert_template_ca_and_more.py (1)
1-151: LGTM!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)
1-149: LGTM!openwisp_controller/config/base/config.py (4)
67-73: LGTM!
185-198: LGTM!
630-674: LGTM!
1161-1161: LGTM!openwisp_controller/config/base/device.py (1)
33-39: LGTM!Also applies to: 303-305, 315-315, 324-352, 354-380, 391-405
openwisp_controller/config/tests/test_admin.py (1)
9-12: LGTM!Also applies to: 38-41, 59-59, 1761-1796, 1798-1849, 1851-1906, 2438-2439
openwisp_controller/config/tests/test_api.py (1)
36-36: LGTM!Also applies to: 559-559, 747-747, 1359-1454, 1456-1510, 1512-1548, 1550-1623
openwisp_controller/config/tests/test_device.py (1)
747-844: LGTM!Also applies to: 847-1071
openwisp_controller/config/tests/test_selenium.py (1)
491-528: LGTM!Also applies to: 846-880
openwisp_controller/config/tests/test_template.py (1)
975-1062: LGTM!Also applies to: 1065-1274, 1276-1438, 1440-1523, 1548-1690
tests/openwisp2/sample_config/models.py (1)
5-5: 🗄️ Data Integrity & IntegrationVerify
extending.rstdocuments the newDeviceCertificateswappable model.A past review noted that adding this model requires updating
docs/developer/extending.rst. That file is not part of this review batch, so the current status cannot be confirmed here.#!/bin/bash fd -e rst . docs | xargs rg -n -i "DeviceCertificate|CONFIG_DEVICECERTIFICATE_MODEL"Also applies to: 99-107
openwisp_controller/config/settings.py (1)
37-39: 📐 Maintainability & Code QualityVerify
docs/user/settings.rstdocumentsOPENWISP_CONTROLLER_REGENERATE_CERTS_ON_HARDWARE_CHANGE.A past review noted this new public setting is undocumented, and that
OPENWISP_CONTROLLER_COMMON_NAME_FORMATdocumentation should also be updated to mention standalone certificate templates.docs/user/settings.rstis not part of this review batch, so current status cannot be confirmed here.#!/bin/bash fd settings.rst docs | xargs rg -n -i "REGENERATE_CERTS_ON_HARDWARE_CHANGE|COMMON_NAME_FORMAT"openwisp_controller/config/utils.py (1)
1-7: LGTM!Also applies to: 16-17, 94-107, 110-144, 147-158
openwisp_controller/config/base/vpn.py (1)
36-41: LGTM!Also applies to: 987-987, 1017-1017, 1035-1042
tests/openwisp2/settings.py (1)
294-295: LGTM!openwisp_controller/config/base/device_certificate.py (1)
1-207: LGTM!Also applies to: 215-246, 255-292
openwisp_controller/config/apps.py (1)
158-169: LGTM!Also applies to: 194-198, 220-224
openwisp_controller/config/tasks.py (1)
223-226: LGTM!openwisp_controller/config/handlers.py (1)
1-8: LGTM!Also applies to: 46-94
openwisp_controller/config/tests/test_config.py (1)
875-885: LGTM!openwisp_controller/config/tests/test_vpn.py (1)
544-565: LGTM!Also applies to: 566-580
openwisp_controller/pki/tests/test_api.py (1)
155-155: LGTM!Also applies to: 247-275, 288-309, 394-394, 405-405, 414-414
| active_device_certs = qs.select_related("cert", "config", "template") | ||
| if not active_device_certs.exists(): | ||
| return | ||
| expected_map = dict(expected_cert_ids) if expected_cert_ids else {} |
There was a problem hiding this comment.
A normal device rename can revoke and reissue the same certificate twice when duplicate regeneration tasks run. Celery serializes these expected certificate IDs as strings, while this map uses UUID instances as keys, so the idempotency check never matches after the task crosses the queue. Please normalize both sides before comparing them and add coverage with JSON-serialized task arguments.
| setattr(self, f"_initial_{field}", getattr(self, field)) | ||
|
|
||
| def save(self, *args, **kwargs): | ||
| super().save(*args, **kwargs) |
There was a problem hiding this comment.
An active certificate template can pass the mutation lock after an unrelated partial save. A dirty CA or blueprint value that was not written is still copied into the initial* snapshot here, so the later validation compares the new value to itself. Please refresh snapshots only for fields in update_fields and add a regression test for an active template.
| "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
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