[feature] Added X.509 Certificate Generator Templates - #1378
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team 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 X.509 certificate-generator templates with CA and optional blueprint certificate relations. Adds relational Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to The certificate-template feature introduces a bounded correctness risk because partial device saves can emit management-IP change events for values that were not persisted and may emit them again later; one concurrency test may also fail to observe setup data reliably. These issues should be fixed or explicitly accepted before merging. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description includes the checklist, linked issues, change summary, manual test plan, and a screenshot reference. It does not explicitly confirm the Anti AI Spam Policy item or use a separate Screenshot heading, but the required information is otherwise substantially complete. Full details: Linked Issues checkExplanation The changes address the linked objectives for certificate template fields and validation [ Full details: Out of Scope Changes checkExplanation The code, tests, migrations, admin/API changes, documentation, UI changes, and VPN refactoring support the linked certificate-template and certificate-lifecycle objectives. No unrelated code changes are evident. The ignored SVG file is not relevant to the linked objectives. Full details: Ui Changes, Regression Test, DocsExplanation PASS: The PR changes the admin UI, JavaScript, CSS, and device template rendering. The description includes a GitHub user-attachment MP4 screen recording, and the manual test plan describes the UI flow and expected before/after states. The feature diff adds regression coverage in ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous Review Summaries (16 snapshots, latest commit 06a0528)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 06a0528)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files, incremental since 570e193)
The concurrency hardening for Fix these issues in Kilo Cloud Previous review (commit 570e193)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit faf5987)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit e74169c)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (1 file, incremental since 8ce1c6b)
The latest commit Fix these issues in Kilo Cloud Previous review (commit 4f4f647)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (1 file, incremental since 8ce1c6b)
The latest commit Fix these issues in Kilo Cloud Previous review (commit 8ce1c6b)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file, incremental since 30ff102)
Previous review (commit 30ff102)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files, incremental since 9d134d6)
Previous review (commit 9d134d6)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files, incremental since c789eca)
Previous review (commit c789eca)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file, incremental since 6727eac)
Previous review (commit 6727eac)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (10 files, incremental since 0a1e011)
The P1 concern raised on Fix these issues in Kilo Cloud Previous review (commit 0a1e011)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files, incremental since 0b0a252)
Previous review (commit 0b0a252)Status: No Issues Found | Recommendation: Merge Files Reviewed (12 files)
Previous review (commit 881f1f8)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 0fb2397)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit dc53718)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 1f6fe30)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Reviewed by balanced · Input: 39.7K · Output: 16.6K · Cached: 877.8K |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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
Added test for the validation branch that now skips ca / blueprint_cert checks for non-cert templates Fixes #1356
Fixed line too long flake error Fixes #1356
Updated test by joining the list of strings into one sentence. Fixes #1377
Fixed both reviews with P2 priority
Fixed atomicity issue in post_clear
…s/1356-extend-abstract-template
Serialized certificate minting with ca renewal
Fixed the uncaught validation error in save.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
@coderabbitai is there any outstanding issue? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openwisp/openwisp-controller"
pr="1378"
echo "=== PR status ==="
gh pr view "$pr" --repo "$repo" \
--json state,isDraft,mergeStateStatus,reviewDecision,headRefOid,statusCheckRollup \
--jq '{
state,
isDraft,
mergeStateStatus,
reviewDecision,
headRefOid,
checks: [.statusCheckRollup[]? | {
name,
status,
conclusion,
workflowName
}]
}'
echo "=== Unresolved review threads ==="
gh api graphql -F owner="openwisp" -F name="openwisp-controller" -F number="$pr" -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
comments(first: 20) {
nodes {
author { login }
path
line
originalLine
body
createdAt
}
}
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes
| map(select(.isResolved == false))
| map({
id,
outdated: .isOutdated,
comments: [.comments.nodes[] | {
author: .author.login,
path,
line,
originalLine,
createdAt,
body
}]
})'Length of output: 25715 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Template serializer API fields ==="
rg -n -A28 -B8 'class TemplateSerializer|blueprint_cert|ca' \
openwisp_controller/config/api/serializers.py
echo "=== Certificate context variables ==="
rg -n -A42 -B8 'def get_cert_context|_path|_key_path' \
openwisp_controller/config/base/config.py
echo "=== Relevant documentation ==="
rg -n -A12 -B6 'certificate-templates|cert_.*(path|key_path)|REGENERATE_CERTS_ON_HARDWARE_CHANGE|COMMON_NAME_FORMAT|type.*cert' \
docs/index.rst \
docs/user/intro.rst \
docs/user/certificate-templates.rst \
docs/user/settings.rst \
docs/user/rest-api.rst \
docs/user/templates.rst
echo "=== Device-admin certificate details ==="
rg -n -A20 -B8 'certificate_details|get_device_certificate_details|device_certificates' \
openwisp_controller/config/admin.py \
openwisp_controller/config/templates/admin/config/device/change_form.html \
openwisp_controller/config/templates/admin/config/device_certificates_table.htmlLength of output: 40735
One non-blocking verification remains. I did not render GitHub still shows 🧠 Learnings usedYou are interacting with an AI system. |
| - **Subject Alternative Names (SAN):** SAN extensions are not | ||
| automatically injected. If a SAN is required, it must be included in the | ||
| blueprint certificate's extensions and will be copied to generated | ||
| certificates. |
There was a problem hiding this comment.
This SAN workaround cannot be used with the supported django-x509 extensions field. subjectAltName is not an accepted named extension, and its standard OID is reserved, so a blueprint containing a SAN fails validation rather than being copied.
Please remove this workaround and state that SANs are unsupported until django-x509 provides a supported representation.
Severity: P3
| When a device's name or MAC address changes, OpenWISP automatically | ||
| revokes the existing active X.509 client certificates generated by | ||
| Certificate Templates and provisions new ones with the updated identity | ||
| attributes. |
There was a problem hiding this comment.
The setting documents only the enabled behavior. Operators choosing False need to know that a later name or MAC change leaves the existing certificate active and does not schedule revocation or reprovisioning.
Please document the disabled behavior explicitly.
Severity: P3
| def _check_management_ip_changed(self, update_fields=None): | ||
| if self._initial_management_ip == models.DEFERRED: | ||
| return | ||
| if self.management_ip != self._initial_management_ip: |
There was a problem hiding this comment.
update_fields is handled for the name and MAC checks, but this signal is still emitted for an unsaved management_ip value. Calling device.save(update_fields=["name"]) after changing management_ip in memory leaves the database unchanged while notifying listeners of a change, and the stale value triggers the same false notification on later partial saves.
Please gate the comparison and signal on update_fields is None or "management_ip" in update_fields, and cover this partial-save case.
Severity: P2
| continue | ||
| dc.template = Template.lock_for_certificate_assignment(dc.template_id) | ||
| old_cert = dc.cert | ||
| old_cert.revoke() |
There was a problem hiding this comment.
The replacement flow locks only DeviceCertificate, not old_cert, while the renewal endpoint can save an already loaded certificate instance. An overlapping renewal can therefore restore the revoked credential:
- The renewal request reads the old certificate while
revoked=False. - This task revokes it and points
DeviceCertificateto the replacement. - The renewal save resumes and writes its stale
revoked=Falsevalue back to the old, now unbound certificate.
Please serialize renewal, revocation, and replacement on the same certificate row, and add a concurrent regression which verifies that the previous credential remains revoked.
Severity: P1
| @@ -0,0 +1,5 @@ | |||
| <svg width="14" height="14" viewBox="0 0 14 14" fill="none" xmlns="http://www.w3.org/2000/svg"> | |||
There was a problem hiding this comment.
This asset is not referenced by the new certificate UI or elsewhere in the repository. The table uses Django admin's admin/img/icon-unknown.svg, so this file is packaged but unused.
Please remove the unused asset.
Severity: P3
Addressed latest maintainer reviews.
|
@coderabbitai check the latest commit (06a0528) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="06a05284a84dd609c8cd1c43eb382452df46f0d2"
echo "=== Commit metadata and changed files ==="
git show --no-ext-diff --format=fuller --name-status "$commit"
echo "=== Commit diff ==="
git diff --no-ext-diff --unified=80 "${commit}^" "$commit" -- .
echo "=== Changed Python file outlines ==="
git diff-tree --no-commit-id --name-only -r "$commit" \
| while IFS= read -r file; do
case "$file" in
*.py)
echo "--- $file"
ast-grep outline "$file" --items all || true
;;
esac
done
echo "=== Current PR status checks ==="
gh pr view 1378 --repo openwisp/openwisp-controller \
--json headRefOid,mergeStateStatus,reviewDecision,statusCheckRollup \
--jq '{
headRefOid,
mergeStateStatus,
reviewDecision,
checks: [.statusCheckRollup[]? | {name, status, conclusion}]
}'Length of output: 60111 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openwisp/openwisp-controller"
sha="06a05284a84dd609c8cd1c43eb382452df46f0d2"
echo "=== Failed CI jobs for commit ==="
gh run list --repo "$repo" --commit "$sha" --limit 30 \
--json databaseId,name,status,conclusion,workflowName,url \
--jq '.[] | select(.conclusion == "failure")'
echo "=== Failure excerpts ==="
gh run list --repo "$repo" --commit "$sha" --limit 30 \
--json databaseId,conclusion \
--jq '.[] | select(.conclusion == "failure") | .databaseId' \
| while IFS= read -r run_id; do
echo "--- run $run_id"
gh run view "$run_id" --repo "$repo" --log-failed 2>&1 | tail -n 220 || true
done
echo "=== Local certificate regeneration paths ==="
rg -n -A50 -B12 \
'def regenerate_certificates|\.renew\(\)|\.revoke\(\)|revoke_device_cert' \
openwisp_controller/config openwisp_controller/pki --glob '*.py'
echo "=== django-x509 1.4 certificate lifecycle methods ==="
gh api 'repos/openwisp/django-x509/contents/django_x509/base/models.py?ref=1.4' \
--jq '.content' \
| base64 --decode \
| nl -ba \
| grep -E -A55 -B12 'def (renew|revoke)\(' || true
echo "=== Related API error-response tests ==="
rg -n -A35 -B12 \
'ValidationError|CertRenewView|cert_renew|post_revoked_cert_renew' \
openwisp_controller/pki/tests openwisp_controller/pki/api --glob '*.py'Length of output: 131389 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openwisp/openwisp-controller"
sha="06a05284a84dd609c8cd1c43eb382452df46f0d2"
echo "=== All GitHub check runs and failure details ==="
gh api "repos/$repo/commits/$sha/check-runs?per_page=100" \
--jq '.check_runs[] | {
id, name, status, conclusion, details_url,
started_at, completed_at,
output: {title: .output.title, summary: .output.summary}
}'
echo "=== django-x509 1.4 lifecycle implementations ==="
gh api 'repos/openwisp/django-x509/contents/django_x509/base/models.py?ref=1.4' \
--jq '.content' \
| base64 --decode \
| awk '
/def revoke\(self\):|def renew\(self\):/ {show=1; count=0}
show {printf "%5d %s\n", NR, $0; count++}
show && count > 100 {show=0}
'
echo "=== Complete local lifecycle and signal context ==="
sed -n '1,185p' openwisp_controller/pki/base/models.py
sed -n '270,365p' openwisp_controller/config/base/device_certificate.py
sed -n '880,950p' openwisp_controller/config/tests/test_device.py
echo "=== Complete changed API test class ==="
sed -n '395,475p' openwisp_controller/pki/tests/test_api.py
echo "=== Image references after help.svg deletion ==="
rg -n -C2 'help\.svg|static.*help|img/help' . \
-g '!*.lock' -g '!*.map' || trueLength of output: 25898 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openwisp/openwisp-controller"
for job_id in 100339854184 100339854207 100339854279; do
echo "=== Job $job_id metadata ==="
gh api "repos/$repo/actions/jobs/$job_id" \
--jq '{name, status, conclusion, started_at, completed_at, html_url, steps}'
echo "=== Job $job_id annotations ==="
gh api "repos/$repo/check-runs/$job_id/annotations?per_page=100" \
--jq '.[] | {path, start_line, end_line, annotation_level, title, message, raw_details}' \
|| true
echo "=== Job $job_id log tail ==="
tmp_log="$(mktemp)"
if gh api -H 'Accept: application/vnd.github+json' \
"repos/$repo/actions/jobs/$job_id/logs" > "$tmp_log" 2>/dev/null; then
tail -n 260 "$tmp_log"
else
echo "Job logs are not available through the API."
fi
rm -f "$tmp_log"
done
echo "=== Declared Django support range ==="
rg -n -C3 'Django|django' pyproject.toml setup.cfg setup.py tox.ini .github \
-g '*.toml' -g '*.cfg' -g '*.py' -g '*.ini' -g '*.yml' -g '*.yaml' || true
echo "=== Complete django-x509 AbstractCert.revoke() source ==="
gh api 'repos/openwisp/django-x509/contents/django_x509/base/models.py?ref=1.4' \
--jq '.content' \
| base64 --decode \
| awk 'NR >= 985 && NR <= 1015 {printf "%5d %s\n", NR, $0}'Length of output: 14073 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openwisp/openwisp-controller"
run_id="33657575549"
for job_id in 100339854184 100339854207 100339854279; do
echo "=== Failed output for job $job_id ==="
gh run view "$run_id" --repo "$repo" --job "$job_id" --log-failed 2>&1 || true
done
echo "=== CI commands at the failed workflow lines ==="
sed -n '40,105p' .github/workflows/ci.yml
echo "=== Commit parent CI comparison ==="
parent_sha="$(git rev-parse 06a05284a84dd609c8cd1c43eb382452df46f0d2^)"
echo "parent=$parent_sha"
gh api "repos/$repo/commits/$parent_sha/check-runs?per_page=100" \
--jq '.check_runs[] | select(.name | contains("django~=4.2.0")) | {name, status, conclusion, details_url}' \
|| trueLength of output: 4057
The certificate locking changes are consistent with the inspected CI currently has a blocking regression:
The failure cause is inconclusive until the workflow completes. Please retry the check after completion so I can inspect the job output. 🧠 Learnings usedYou are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="06a05284a84dd609c8cd1c43eb382452df46f0d2"
echo "=== Commit metadata and changed files ==="
git show --no-ext-diff --format=fuller --name-status "$commit"
echo "=== Commit diff ==="
git show --no-ext-diff --format= --find-renames --find-copies "$commit"
echo "=== Structure of changed Python files ==="
git diff-tree --no-commit-id --name-only -r "$commit" \
| rg '\.py$' \
| while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
doneLength of output: 24817 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Local certificate model and regeneration paths ==="
sed -n '1,190p' openwisp_controller/pki/base/models.py
rg -n -A32 -B12 '\.(renew|revoke)\s*\(|def regenerate_certificates|revoke_device_cert' \
openwisp_controller/config openwisp_controller/pki --glob '*.py'
echo "=== django-x509 AbstractCert implementation available in the checkout ==="
fd -HI 'models.py' . \
| while IFS= read -r file; do
if rg -q 'class AbstractCert\b' "$file"; then
echo "--- $file"
rg -n -A110 -B10 'class AbstractCert\b|^\s+def (renew|revoke)\b' "$file"
fi
done
echo "=== Certificate API exception handling and serializer contract ==="
sed -n '80,125p' openwisp_controller/pki/api/views.py
rg -n -A24 -B8 'class CertRevokeRenewSerializer|ValidationError|exception_handler' \
openwisp_controller/pki --glob '*.py'
echo "=== Removed asset references ==="
rg -n -i -C2 'help\.svg|img/help|help-icon' . \
-g '!**/.git/**' || true
echo "=== Management-IP signal implementation and coverage ==="
rg -n -A24 -B10 'management_ip_changed|_check_management_ip_changed|management_ip.*update_fields' \
openwisp_controller --glob '*.py'Length of output: 145505 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Declared django-x509 dependency ==="
rg -n -i -C2 'django[-_]?x509' \
pyproject.toml setup.cfg setup.py requirements.txt requirements*.txt 2>/dev/null || true
echo "=== django-x509 1.4 certificate methods ==="
gh api 'repos/openwisp/django-x509/contents/django_x509/base/models.py?ref=1.4' \
--jq '.content' \
| base64 --decode \
| nl -ba \
| sed -n '/class AbstractCert/,/^class /p' \
| sed -n '1,260p'
echo "=== Local certificate save/revoke/renew receivers ==="
rg -n -A30 -B10 \
'`@receiver`\(.*(Cert|cert)|post_save.*Cert|pre_save.*Cert|post_delete.*Cert|cert_.*(saved|modified|revoked)|def .*cert.*(save|revoke|renew)' \
openwisp_controller --glob '*.py'Length of output: 2352 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== django-x509 1.4 AbstractCert ==="
gh api 'repos/openwisp/django-x509/contents/django_x509/base/models.py?ref=1.4' \
--jq '.content' \
| base64 --decode \
| awk '
/class AbstractCert/ { capture=1 }
capture { printf "%6d %s\n", NR, $0 }
capture && NR > start + 320 && /^class / { exit }
'
echo "=== django-x509 1.4 revoke and renew definitions ==="
gh api 'repos/openwisp/django-x509/contents/django_x509/base/models.py?ref=1.4' \
--jq '.content' \
| base64 --decode \
| awk '
/^[[:space:]]+def (revoke|renew)\(/ { from=NR-10; to=NR+100 }
NR >= from && NR <= to { printf "%6d %s\n", NR, $0 }
'
echo "=== Certificate model signal registrations and receivers ==="
rg -n -A30 -B10 \
'post_save|pre_save|post_delete|m2m_changed|update_related_config_status|cert.*(modified|renew|revoke)' \
openwisp_controller/config openwisp_controller/pki --glob '*.py'Length of output: 201481 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh api 'repos/openwisp/django-x509/contents/django_x509/base/models.py?ref=1.4' \
--jq '.content' \
| base64 --decode \
| awk '
/^class AbstractCert\b/ { in_cert=1 }
in_cert && /^ def revoke\(/ { show=1; remaining=70 }
in_cert && /^ def renew\(/ { show=1; remaining=100 }
show { printf "%6d %s\n", NR, $0; remaining-- }
show && remaining == 0 { show=0 }
'
echo "=== Local model save path used after django-x509 renewal or revocation ==="
sed -n '106,180p' openwisp_controller/pki/base/models.py
echo "=== Current commit status checks ==="
gh pr view 1378 --repo openwisp/openwisp-controller \
--json headRefOid,statusCheckRollup \
--jq '{headRefOid, checks: [.statusCheckRollup[]? | {name, status, conclusion}]}'Length of output: 4921 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openwisp/openwisp-controller"
sha="06a05284a84dd609c8cd1c43eb382452df46f0d2"
echo "=== Failed workflow runs for the checked commit ==="
gh run list --repo "$repo" --commit "$sha" --limit 100 \
--json databaseId,name,displayTitle,workflowName,status,conclusion,url \
--jq '.[] | select(.conclusion == "failure")'
echo "=== Failed job logs ==="
for run_id in $(gh run list --repo "$repo" --commit "$sha" --limit 100 \
--json databaseId,conclusion \
--jq '.[] | select(.conclusion == "failure") | .databaseId'); do
echo "--- Run: $run_id"
gh run view "$run_id" --repo "$repo" --log-failed || true
done
echo "=== django-x509 1.4 AbstractCert renewal/revocation methods ==="
gh api 'repos/openwisp/django-x509/contents/django_x509/base/models.py?ref=1.4' \
--jq '.content' \
| base64 --decode \
| awk '
/^[[:space:]]*def revoke\(/ || /^[[:space:]]*def renew\(/ { remaining=90 }
remaining > 0 { printf "%6d %s\n", NR, $0; remaining-- }
'Length of output: 5768 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openwisp/openwisp-controller"
sha="06a05284a84dd609c8cd1c43eb382452df46f0d2"
echo "=== Django 4.2 check-run details ==="
gh api "repos/$repo/commits/$sha/check-runs?per_page=100" \
--jq '
.check_runs[]
| select(.name | test("django~=4\\.2\\.0"; "i"))
| {
name,
status,
conclusion,
started_at,
completed_at,
details_url,
output: {
title: .output.title,
summary: .output.summary,
text: .output.text
},
app: .app.slug
}
'
echo "=== All non-success check runs on this commit ==="
gh api "repos/$repo/commits/$sha/check-runs?per_page=100" \
--jq '
.check_runs[]
| select(.status != "completed" or (.conclusion != "success" and .conclusion != "skipped" and .conclusion != "neutral"))
| {name, status, conclusion, details_url, app: .app.slug, output: .output}
'Length of output: 4564 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openwisp/openwisp-controller"
run_id="33657575549"
echo "=== Failed Django 4.2 check annotations ==="
for check_id in 100339854279 100339854207 100339854184; do
echo "--- Check run: $check_id"
gh api "repos/$repo/check-runs/$check_id/annotations?per_page=100" \
--jq '.[] | {path, start_line, end_line, annotation_level, title, message, raw_details}' || true
done
echo "=== Workflow jobs ==="
gh api "repos/$repo/actions/runs/$run_id/jobs?per_page=100" \
--jq '.jobs[] | select(.conclusion == "failure") | {id, name, status, conclusion, steps: [.steps[] | select(.conclusion == "failure") | {name, number, conclusion}]}'
echo "=== Failed job logs ==="
for job_id in 100339854279 100339854207 100339854184; do
echo "--- Job: $job_id"
gh api "repos/$repo/actions/jobs/$job_id/logs" 2>/dev/null \
| rg -n -i -C4 'error|failure|failed|traceback|assertionerror|exception' \
|| true
doneLength of output: 2901 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Workflow files and the annotated commands ==="
fd -HI . .github 2>/dev/null || true
rg -n -A12 -B8 'QA checks|Tests|pytest|ruff|black|isort|flake8' .github --glob '*.{yml,yaml}'
echo "=== Failed workflow output ==="
gh run view 33657575549 --repo openwisp/openwisp-controller --log-failed 2>&1 || true
echo "=== Job-log HTTP response metadata ==="
gh api -i 'repos/openwisp/openwisp-controller/actions/jobs/100339854279/logs' 2>&1 \
| sed -n '1,80p' || trueLength of output: 3949 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== QA commands ==="
sed -n '1,240p' run-qa-checks
echo "=== Test runner command ==="
sed -n '1,180p' runtests
echo "=== Static diff checks ==="
git diff --check 06a05284a84dd609c8cd1c43eb382452df46f0d2^ \
06a05284a84dd609c8cd1c43eb382452df46f0d2
echo "=== Failed Django 4.2 job logs ==="
for job_id in 100339854279 100339854207 100339854184; do
echo "--- Job: $job_id"
gh api --allow-escape-sequences \
"repos/openwisp/openwisp-controller/actions/jobs/$job_id/logs" \
| sed -r 's/\x1B\[[0-9;]*[[:alpha:]]//g' \
| rg -n -i -C5 \
'error|failure|failed|traceback|assertionerror|exception|would reformat|not formatted' \
|| true
doneLength of output: 29578
CI has a separate blocker. All Django 4.2 jobs fail before the test suite runs. Dependency resolution installs Some Django 5.1 and 5.2 jobs were still running during the check. I also used the existing certificate-lifecycle context for this PR. 🧠 Learnings usedYou are interacting with an AI system. |
Django System Check Error: Choices Must Be an IterableHello @stktyagi,
|
Django System Check Choice Field ErrorsHello @stktyagi,
|
Django system check errors on model choicesHello @stktyagi,
|
Django System Check Error: Choices Must Be IterableHello @stktyagi,
|
…s/1356-extend-abstract-template
Fixed unused import error.
Fix unused import in
|
| mocked_task.assert_called_once() | ||
|
|
||
|
|
||
| class TestDeviceCertificateRegenerationTaskTransaction( |
There was a problem hiding this comment.
does this test really require a new TransactionTestCase class? Each of this task makes the tests slower, please check if it can be added to an existing class.
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