[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 Sequence Diagram(s)sequenceDiagram
participant Admin
participant DeviceAPI
participant Config
participant DeviceCertificate
participant PKI
Admin->>DeviceAPI: assign cert template
DeviceAPI->>Config: update templates
Config->>DeviceCertificate: create relationship
DeviceCertificate->>PKI: generate certificate
PKI-->>DeviceCertificate: return certificate
DeviceCertificate-->>Config: update configuration status
Possibly related issues
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (5 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 (11 files)
Previous Review Summaries (16 snapshots, latest commit c09b330)Current summary above is authoritative. Previous snapshots are kept for context only. 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 (
No production request handlers, serializers, migrations, tenant-filtering, or security-relevant code paths are modified in this delta. No new critical bugs or security vulnerabilities. Files Reviewed (incremental, 2 files)
Previous review (commit 52c7544)Status: No Issues Found | Recommendation: Merge Incremental Review (since b486eb5)The new commit (
No production request handlers, serializers, migrations, or tenant-filtering code paths are changed in this delta. No new critical bugs or security vulnerabilities were introduced. Files Reviewed (incremental, 3 files)
Previous review (commit b486eb5)Status: No Issues Found | Recommendation: Merge Incremental Review (since b52fe63)The new commits are low-risk follow-ups:
All changes are confined to the test Files Reviewed (incremental)
Previous review (commit b52fe63)Status: No Issues Found | Recommendation: Merge Incremental Review (since fb54051)The new commit A corresponding regression test, No new critical bugs or security vulnerabilities were introduced. Files Reviewed (incremental)
Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by step-3.7-flash · Input: 315.7K · Output: 13.1K · Cached: 3.4M |
|
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
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openwisp_controller/config/api/serializers.py (1)
236-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequired-but-omitted cert templates still get unnecessarily revoked/regenerated.
config_templateshere is the raw list submitted by the client and does not include required templates thatenforce_required_templateswill force back in during the subsequenttemplates.set(..., clear=True)cycle. If a client omits a requiredcert-type template from the submitted list, itsDeviceCertificateis deleted (revoking the underlying cert) by thisexclude(...).delete()before the required-template re-add restores the template andmanage_device_certs'spost_addbranch regenerates a brand-new certificate for it. The template stays assigned, but the certificate is churned unnecessarily.This matches a previously raised concern that a fix should either wait until the final template set (including required templates) is known, or rely solely on the
manage_device_certsM2M lifecycle handler for this cleanup.💡 Possible direction
- DeviceCertificate.objects.filter(config=config).exclude( - template_id__in=config_templates - ).delete() + required_ids = list( + config.templates.filter( + required=True, backend=config.backend + ).values_list("id", flat=True) + ) + DeviceCertificate.objects.filter(config=config).exclude( + template_id__in=config_templates + required_ids + ).delete() config.templates.set(config_templates, clear=True)🤖 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 236 - 251, Update the cleanup logic in the _prepare_config/template assignment flow so DeviceCertificate deletion uses the final template set after required templates are enforced, rather than the raw config_templates submission. Alternatively, remove this manual certificate cleanup and rely on the manage_device_certs M2M lifecycle handler, ensuring omitted required cert templates remain assigned without certificate revocation or regeneration.
🤖 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/device.py`:
- Around line 371-382: Gate the side-effects in _check_name_changed and
_check_mac_address_changed with _field_was_saved for their respective fields
before emitting device_name_changed or calling config.set_status_modified().
Preserve the existing _initial_* updates conditioned on update_fields, and
ensure partial saves do not react to dirty in-memory values for fields that were
not persisted.
In `@openwisp_controller/config/base/template.py`:
- Around line 74-84: Update the ca ForeignKey in the certificate template model
to use models.PROTECT instead of models.CASCADE, preventing deletion of a
referenced CA and preserving the active-template mutation safeguards.
---
Duplicate comments:
In `@openwisp_controller/config/api/serializers.py`:
- Around line 236-251: Update the cleanup logic in the _prepare_config/template
assignment flow so DeviceCertificate deletion uses the final template set after
required templates are enforced, rather than the raw config_templates
submission. Alternatively, remove this manual certificate cleanup and rely on
the manage_device_certs M2M lifecycle handler, ensuring omitted required cert
templates remain assigned without certificate revocation or regeneration.
🪄 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: dbd8fa15-5eb1-444d-88b0-01b1026d43c3
⛔ Files ignored due to path filters (1)
openwisp_controller/config/static/config/img/help.svgis excluded by!**/*.svg
📒 Files selected for processing (36)
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/0065_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/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0011_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. (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | 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.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/config/migrations/0065_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/apps.pytests/openwisp2/sample_config/migrations/0011_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/models.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_certificate.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/0065_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/templates/admin/config/device_certificates_table.htmlopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/templates/admin/config/device/change_form.htmlopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/apps.pytests/openwisp2/sample_config/migrations/0011_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/models.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_certificate.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/migrations/0065_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/models.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_certificate.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/0065_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/models.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_certificate.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/0065_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/apps.pytests/openwisp2/sample_config/migrations/0011_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/utils.pyopenwisp_controller/config/models.pyopenwisp_controller/config/base/vpn.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device_certificate.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_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/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.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/sample_config/migrations/0011_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/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.pyopenwisp_controller/config/tests/test_config.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/sample_config/migrations/0011_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/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_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/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_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/pki/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/0011_template_blueprint_cert_template_ca_and_more.py
🪛 ast-grep (0.44.1)
openwisp_controller/config/tasks.py
[warning] 223-223: 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)
tests/openwisp2/sample_config/migrations/0011_template_blueprint_cert_template_ca_and_more.py
[info] 106-106: 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/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/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/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] 313-313: 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] 378-378: 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)
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] 163-163: 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)
🪛 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 (38)
openwisp_controller/config/base/vpn.py (1)
36-41: LGTM!Also applies to: 987-987, 1017-1017, 1041-1042
openwisp_controller/config/base/device.py (1)
33-39: LGTM!Also applies to: 303-303, 313-313, 322-328, 393-407
openwisp_controller/config/handlers.py (1)
46-93: LGTM!openwisp_controller/config/settings.py (1)
37-39: LGTM!openwisp_controller/config/tests/test_template.py (1)
974-1589: LGTM!openwisp_controller/config/tests/test_vpn.py (1)
558-558: LGTM!Also applies to: 575-575
openwisp_controller/pki/tests/test_api.py (1)
247-308: LGTM!openwisp_controller/config/tests/test_api.py (1)
1340-1547: LGTM!openwisp_controller/config/tests/test_config.py (1)
878-885: LGTM!openwisp_controller/config/tests/test_device.py (1)
747-1176: LGTM!openwisp_controller/config/tests/test_selenium.py (1)
491-880: LGTM!openwisp_controller/config/tests/test_admin.py (1)
1824-1856: LGTM!Also applies to: 2388-2389
openwisp_controller/config/base/template.py (1)
9-9: LGTM!Also applies to: 25-29, 40-51, 85-96, 133-134, 148-178, 235-244, 294-397, 406-410, 424-433
openwisp_controller/config/migrations/0065_template_blueprint_cert_template_ca_and_more.py (1)
1-152: LGTM!openwisp_controller/config/models.py (1)
5-5: LGTM!Also applies to: 97-106
tests/openwisp2/sample_config/models.py (1)
5-5: LGTM!Also applies to: 99-107
tests/openwisp2/settings.py (1)
284-284: LGTM!openwisp_controller/config/base/device_certificate.py (1)
1-222: LGTM!openwisp_controller/config/tasks.py (1)
220-225: LGTM!docs/developer/extending.rst (1)
345-345: LGTM!docs/index.rst (1)
40-40: LGTM!docs/user/certificate-templates.rst (1)
1-291: LGTM!docs/user/intro.rst (1)
38-39: LGTM!docs/user/rest-api.rst (1)
1115-1115: LGTM!docs/user/templates.rst (1)
212-229: 📐 Maintainability & Code Quality | ⚡ Quick winConfirm
certificate-templates.rstis included in the docs toctree.This section now correctly links to
/controller/user/certificate-templates, addressing part of the earlier feedback. The other half of that feedback - adding the new page to the toctree indocs/index.rstso it's reachable from the built docs navigation - can't be verified from the files in this batch.#!/bin/bash rg -n "certificate-templates" docs/index.rsttests/openwisp2/sample_config/migrations/0011_template_blueprint_cert_template_ca_and_more.py (1)
1-151: LGTM!openwisp_controller/config/utils.py (1)
94-158: LGTM!openwisp_controller/config/base/config.py (2)
44-73: LGTM!Also applies to: 630-676, 1106-1134, 1162-1162
179-198: 📐 Maintainability & Code QualityDrop the duplicate-handler concern — there’s no separate
certificate_updatedreference in the codebase, so_resolve_cert_dependencyis the only resolver here and the duplicate/dead-code warning doesn’t apply.> Likely an incorrect or invalid review comment.openwisp_controller/config/apps.py (1)
158-224: LGTM!openwisp_controller/config/api/serializers.py (1)
20-20: LGTM!Also applies to: 42-62, 75-94, 345-376
openwisp_controller/config/admin.py (2)
23-23: LGTM!Also applies to: 64-64, 121-125, 928-929, 1129-1130, 1144-1153, 1166-1166
917-1025: 📐 Maintainability & Code QualityNo duplicate certificate details block
ConfigInline.certificate_detailsis gone, and certificate details are rendered once throughDeviceAdmin.get_extra_context()andchange_form.html.> Likely an incorrect or invalid review comment.openwisp_controller/config/static/config/js/switcher.js (1)
4-62: LGTM!openwisp_controller/config/templates/admin/config/device/change_form.html (1)
13-28: LGTM!openwisp_controller/config/templates/admin/config/device_certificates_table.html (1)
1-69: LGTM!openwisp_controller/config/static/config/css/admin.css (1)
9-9: LGTM!Also applies to: 401-426
docs/user/settings.rst (1)
300-352: LGTM!Also applies to: 365-381
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Addressed review generated by coderabbit
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Linked template name field and made sure each link opens in new tab
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Can we prevent the creation of certificate generator templates which have the auto_cert flag disabled? I don't see any use case for this.
From a UX perspective, we should hide this field when certificate generator option is selected.
In the model, we should raise ValidationError if it's set to True.
| configuration, ensuring the device pulls the renewed certificate on its | ||
| next check-in. | ||
|
|
||
| **On device property changes :** When a device's **hostname** or **MAC |
There was a problem hiding this comment.
| **On device property changes :** When a device's **hostname** or **MAC | |
| **On device property changes:** When a device's **hostname** or **MAC |
| blueprint certificate's extensions and will be copied to generated | ||
| certificates. | ||
| - **OCSP / CRL distribution points:** These must be configured at the CA | ||
| level in ``django-x509`` and are not managed per-template. |
There was a problem hiding this comment.
CRL are automatically managed by OpenWISP, there's a view which allows to download the automatically genereted CRL which contains the revoked certs, so this line is misleading, by reading it, a user could think we don't do CRLs.
| private CAs managed within OpenWISP's PKI module. | ||
| - **Certificate expiration management:** Auto-renewal upon expiry is not | ||
| built in; renewal must be triggered manually through the admin or API | ||
| endpoint. |
There was a problem hiding this comment.
Please remove this, we want to add this and we may forget this line of docs here which will contradict.
| - **Certificate expiration management:** Auto-renewal upon expiry is not | ||
| built in; renewal must be triggered manually through the admin or API | ||
| endpoint. | ||
| - **Non-``auto_cert`` certificates:** If ``auto_cert`` is disabled, no |
There was a problem hiding this comment.
Is there any use case for this? If not, we should just prevent this completely in validation, I wouldn't even mention this here.
| .. note:: | ||
|
|
||
| The regeneration only applies to certificates created via Certificate | ||
| Templates with ``auto_cert`` enabled. |
| Certificate Templates | ||
| --------------------- | ||
|
|
||
| A Certificate Template is a :doc:`Template </controller/user/templates>` |
There was a problem hiding this comment.
There's no point in linking the templates page again (we're already here).
|
|
||
| .. _certificate_templates: | ||
|
|
||
| Certificate Templates |
There was a problem hiding this comment.
| Certificate Templates | |
| X.509 Certificate Generator Templates |
| Certificate Templates are useful for any use case in which a fleet of | ||
| devices needs an X.509 client certificate managed centrally by OpenWISP, | ||
| for example: mutual TLS authentication against an internal service, signed | ||
| device identities for captive portals, etc. |
There was a problem hiding this comment.
This section looks AI slop to me.
What we should do here is simply say something like:
This template type automatically provisions and revokes X.509 certificates for assigned devices. For details, see the X.509 Certificate Generator Templates documentation.
Keep the details in the dedicated page.
| <tr class="{% cycle 'row1' 'row2' %}"> | ||
| <th class="field-name" scope="row"> | ||
| {% if cert.has_cert %} | ||
| <a href="{{ cert.url }}" target="_blank" style="text-decoration: none;">{{ cert.common_name }}</a> |
There was a problem hiding this comment.
Using inline CSS in the UI is an anti-pattern because it makes maintenance harder.
| blank=True, | ||
| null=True, | ||
| ) | ||
| auto_cert = models.BooleanField(default=False) |
Consistency, Usability, Accessibility.
| @@ -341,6 +369,10 @@ def update(self, instance, validated_data): | |||
| pk_set=None, | |||
There was a problem hiding this comment.
The transaction at this block is not wide enough. It commits the old DeviceCertificate deletion and template replacement before _update_config() and super().update() finish validating and saving the request. A combined organization and config update can therefore return 400 for an invalid template after the old certificates have already been revoked and the new organization required templates have been attached, while the device still belongs to the old organization. Wrap the complete update in one transaction and add a regression test which verifies that all these changes are rolled back when config validation fails.
| cert_name = self.cert.name if self.cert else str(_("Pending Generation")) | ||
| return f"{self.config.device.name} - {cert_name}" | ||
|
|
||
| def clean(self): |
There was a problem hiding this comment.
This validation only prevents a blueprint certificate from being reused. A DeviceCertificate can still be saved with a template which is not assigned to the config, a non-certificate template, or a certificate owned by another organization or signed by another CA. get_cert_context() will then expose that certificate and its private key to the device. Validate the complete relationship here: the template must be an assigned certificate generator, the config/template organizations must be compatible, and a supplied certificate must be non-revoked and match the device organization and template CA. Cover the cross-organization case because this is an organization boundary.
| @@ -161,9 +232,16 @@ def _update_related_config_status(self): | |||
| # use atomic to ensure any code bound to | |||
There was a problem hiding this comment.
These snapshots are initialized once but are not updated after a successful save or refresh_from_db(). For example, I can change an unassigned template from CA-1 to CA-2 and save it, assign the same instance to a config, then change it back to CA-1. _initial_ca_id is still CA-1, so _validate_cert_template_changes() sees no change and allows mutation of an active template. Refresh the saved snapshots after successful saves and refreshes, with a regression test using the same model instance across both saves.
| """ | ||
| if not app_settings.REGENERATE_CERTS_ON_HARDWARE_CHANGE: | ||
| return | ||
| Device = load_model("config", "Device") |
There was a problem hiding this comment.
The device is read before the transaction and is never locked. If task A reads identity A, then a second save writes identity B while task A holds only the DeviceCertificate lock, both queued tasks can expect the old certificate. Task A installs a certificate for identity A and task B then skips because that expected certificate ID is no longer current. The device remains at identity B with a certificate for A. Select the device for update inside this transaction before locking its certificate rows. Also keep the checksum/status updates inside the same transaction: at present the rotation commits at the end of the atomic block, so a rendering failure or worker exit before the post-commit status update leaves the old certificate revoked and checksum_db stale, and retrying with the old expected ID cannot recover it. Add overlap and failure-rollback tests.
| template = models.ForeignKey( | ||
| get_model_name("config", "Template"), on_delete=models.CASCADE | ||
| ) | ||
| cert = models.OneToOneField( |
There was a problem hiding this comment.
on_delete=CASCADE allows a generated certificate to be deleted directly from PKI while its certificate template remains assigned. This cascades the DeviceCertificate row, does not generate a replacement, and does not update the config checksum, so an applied device loses the certificate context. Block deletion of certificates used by DeviceCertificate from both admin and API, or implement a complete replacement/status-update lifecycle. Add coverage for direct certificate deletion. CA deletion follows a distinct template-cascade lifecycle and needs separate analysis.
| DeviceCertificate.objects.filter(config=config).exclude( | ||
| template_id__in=config_templates | ||
| ).delete() | ||
| config.templates.set(config_templates, clear=True) |
There was a problem hiding this comment.
config_templates is the raw submitted list, not the final list after required templates are enforced. Omitting a required certificate template deletes its DeviceCertificate, revokes its certificate, then re-adds the same required template and generates a replacement. Remove this early deletion and let manage_device_certs reconcile the final M2M set. The regression test should assert that both the DeviceCertificate and certificate IDs remain unchanged.
| and exposes them as UUID-namespaced variables for the configuration engine. | ||
| """ | ||
| cert_context = collections.OrderedDict() | ||
| if not any(t.type == "cert" for t in self.templates.all()): |
There was a problem hiding this comment.
The first check only proves that at least one certificate template is assigned, but the following loop renders every DeviceCertificate for the config. Removed optional rows remain until the on_commit cleanup, so rendering or explicitly saving the config inside that transaction can include a stale certificate and its key. Filter this queryset to templates currently assigned to the config and use the same queryset for the empty check. Add a required-plus-optional regression test which renders inside the transaction and after commit.
| "by this template." | ||
| ), | ||
| ) | ||
| blueprint_cert = models.ForeignKey( |
There was a problem hiding this comment.
SET_NULL allows the blueprint of an active template to be deleted without running Template.clean(). Existing devices keep certificates cloned from the old blueprint while later assignments silently fall back to CA defaults, which is exactly the inconsistency the active mutation lock is intended to prevent. Restrict direct deletion while a blueprint is referenced by an active template. RESTRICT may preserve the intended full CA cascade while preventing this direct deletion path. Cover active and unassigned templates.
| @@ -303,7 +310,7 @@ def save(self, *args, **kwargs): | |||
| # after performing the save operation. Hence, the actual value | |||
There was a problem hiding this comment.
The deferred-field fallback runs after super().save(), but the regeneration receiver runs during that save. If a device is loaded with .only(id), then name or mac_address is assigned and saved, the receiver still sees the corresponding initial* value as DEFERRED and skips regeneration. The later refresh reads the already-updated database value, so the change is lost permanently and the normal config-status check is skipped too. Load the persisted initial value before saving a newly populated deferred tracked field, and add separate name and MAC regression tests.
| "by this template." | ||
| ), | ||
| ) | ||
| blueprint_cert = models.ForeignKey( |
There was a problem hiding this comment.
SET_NULL allows the blueprint of an active template to be deleted without running Template.clean(). Existing devices keep certificates cloned from the old blueprint while later assignments silently fall back to CA defaults, which is exactly the inconsistency the active mutation lock is intended to prevent. Restrict direct deletion while a blueprint is referenced by an active template. RESTRICT may preserve the intended full CA cascade while preventing this direct deletion path. Cover active and unassigned templates.
| @@ -221,7 +403,11 @@ def clean(self, *args, **kwargs): | |||
| * clears VPN specific fields if type is not VPN | |||
There was a problem hiding this comment.
The exception handler says it will continue, but templates.add() performs the M2M work in an inner atomic(savepoint=False) block. If certificate generation fails for one config, catching the exception inside this outer transaction leaves the transaction marked for rollback. Earlier assignments are rolled back and subsequent database work cannot continue safely. Put each config.templates.add(self) in its own nested savepoint and catch the exception outside that savepoint. A test should make one config fail and verify that the other eligible configs still receive the template.
| self.assertContains(response, device_1.name) | ||
| self.assertContains(response, cert_1.name) | ||
| self.assertNotContains(response, cert_2.name) | ||
| cert_admin = admin.site.get_model_admin(Cert) |
There was a problem hiding this comment.
Latest changes summary: the Device admin now renders a standalone X.509 certificate table and links to the certificate changelist filtered by device. The certificate changelist has a device-name autocomplete filter immediately after the organization filter. Both autocomplete results and selected labels use the request-scoped Device admin queryset, preventing cross-organization device disclosure. Certificate-admin coverage now verifies filtering by device and verifies that an organization administrator cannot autocomplete or filter a device from another organization. The test helper refactor is previous technical debt, not PR #1378 product behavior. GeoConfig.ready() previously imported Config admin test code under TESTING, resolving Django default admin site before the admin theme could install OpenwispAdminSite. The shared helper now discovers management-form fields from the registered Device admin at test runtime, eliminating that cross-app startup dependency.
This will have to be pushed to master too. There's an old mess in the test code which prevented some new tests to run properly.
c09b330 to
5fc22c3
Compare
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