Skip to content

[feature] Added X.509 Certificate Generator Templates - #1378

Open
stktyagi wants to merge 115 commits into
gsoc26-x509-certificate-generator-templatesfrom
issues/1356-extend-abstract-template
Open

[feature] Added X.509 Certificate Generator Templates#1378
stktyagi wants to merge 115 commits into
gsoc26-x509-certificate-generator-templatesfrom
issues/1356-extend-abstract-template

Conversation

@stktyagi

@stktyagi stktyagi commented May 26, 2026

Copy link
Copy Markdown
Member

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

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

  • Go to PKI -> Certification Authorities and create two CAs: CA-1 and CA-2.
  • Go to PKI -> Certificates and create two certificates to act as blueprints:
  • Blueprint-1 (Must use CA-1)
  • Blueprint-2 (Must use CA-2)
  • Go to Devices and create a device (test-device).

Template Creation and Validation

  • Configuration -> Templates and click ADD TEMPLATE.
  • Set Type to Certificate.
    • Leave CA blank and try to save.
    • Expected Result: Validation error stating a CA is required.
  • Set CA to CA-1.
    • Set Blueprint to Blueprint-2 (which belongs to CA-2). Try to save.
    • When opening drop-down for blueprint you'll only see unassigned and unrevoked certificates.
    • Expected Result: Validation error stating the Blueprint must match the selected CA.
  • Change Blueprint to Blueprint-1. Name the template Active-Cert-Template. Save it.

Device Provisioning

  • Add configuration for test-device.
  • In the templates field, add Active-Cert-Template. Save.
  • Go to PKI -> Certificates.
  • Expected Result: You should see a brand new certificate automatically generated for test-device. Its status should be valid (not revoked).

Active Mutation Locks

  • Go back to Configuration -> Templates and edit Active-Cert-Template (which is now assigned to an active device).
  • Change the Type to Generic. Try to save.
  • Expected Result: Validation error: "You cannot change the template type from certificate on an active template."
  • Change the CA to CA-2. Try to save.
  • Expected Result: Validation error blocking the CA change.
  • Change the Blueprint to Blueprint-2 (ensure you also change the CA so they match, triggering the active lock). Try to save.
  • Expected Result: Validation error blocking the Blueprint change.

Revocation on Removal

  • Go to the Configuration for test-device.
  • Remove Active-Cert-Template entirely from the templates list. Save.
  • Go to PKI -> Certificates and locate the device's certificate.
  • Expected Result: The certificate should still exist in the database, but its status should now be marked as Revoked.

Context Configuration Injection

  • Go to Configuration -> Templates, open Active-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:

    {
        "files": [
            {
                "path": "{{ cert_<uuid>_path }}",
                "mode": "0600",
                "contents": "{{ cert_<uuid>_pem }}"
            }
        ]
    }
    

    (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 the Preview configuration button.

  • 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

…1356

- Added 'cert' to TYPE_CHOICES.
- Introduced 'ca' and 'blueprint_cert' ForeignKeys with organization validation.
- Updated the clean() method to clear unneeded relations, require a CA for cert types, and validate that a blueprint certificate is not already assigned to a device.

Fixes #1356
@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • ai-review
🚫 Excluded labels (none allowed) (1)
  • invalid

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: f42dd832-840f-4193-a8d8-32ca9eab47d1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds standalone X.509 certificate-generator templates with CA and optional blueprint certificate relations. Adds relational DeviceCertificate bindings for generation, assignment, renewal, revocation, and configuration context injection. Adds API, admin, autocomplete, device-page, and template-form support. Device name or MAC changes can trigger asynchronous certificate replacement and notifications. Organization and mutation protections, migrations, tests, and documentation are included.

Estimated code review effort: 5 (Critical) | ~120 minutes

Merge Risk: 🟡 Moderate · up to 30ff1

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: nemesifier, pandafy

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required [feature] prefix and clearly describes the standalone X.509 Certificate Generator Templates added by the pull request.
Description check ✅ Passed 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 Scree…
Linked Issues check ✅ Passed The changes address the linked objectives for certificate template fields and validation [#1356], DeviceCertificate modeling [#1377], admin integration [#1357], REST API support [#1361], certificate l…
Out of Scope Changes check ✅ Passed 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 a…
Ui Changes, Regression Test, Docs ✅ Passed 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 flo…
Full details: Description check

Explanation

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 check

Explanation

The changes address the linked objectives for certificate template fields and validation [#1356], DeviceCertificate modeling [#1377], admin integration [#1357], REST API support [#1361], certificate lifecycle management [#1358], UUID-namespaced configuration variables [#1360], and hardware-change regeneration [#1359]. Tests and documentation cover the stated behavior.

Full details: Out of Scope Changes check

Explanation

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, Docs

Explanation

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 test_template.py, test_api.py, test_device.py, test_admin.py, and Selenium tests for certificate fields and provisioning. The PR also adds docs/user/certificate-templates.rst and updates the templates, settings, REST API, introduction, and documentation index.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issues/1356-extend-abstract-template

Comment @coderabbitai help to get the list of available commands.

@stktyagi

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kilo-code-bot

kilo-code-bot Bot commented May 26, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (6 files)
  • .github/ISSUE_TEMPLATE/bug_report.md
  • .github/workflows/ci.yml
  • openwisp_controller/config/admin.py
  • openwisp_controller/config/tests/test_admin.py
  • openwisp_controller/connection/base/models.py
  • openwisp_controller/connection/tests/test_selenium.py
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

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
openwisp_controller/config/base/device.py 396 Behavior fix lacks a regression test for the dirty-management_ip partial-save case
Files Reviewed (8 files, incremental since 570e193)
  • docs/user/certificate-templates.rst
  • docs/user/settings.rst
  • openwisp_controller/config/base/device.py - 1 issue
  • openwisp_controller/config/static/config/img/help.svg (deleted, unreferenced asset)
  • openwisp_controller/config/tests/test_device.py
  • openwisp_controller/pki/api/views.py
  • openwisp_controller/pki/base/models.py
  • openwisp_controller/pki/tests/test_api.py

The concurrency hardening for Cert.revoke()/renew() (row locks, idempotent revoke, renew-of-revoked guard surfacing as a 400 with a new regression test), the management_ip_changed update_fields gating, and the docs updates look correct and consistent with repository patterns. The only gap: the _check_management_ip_changed() fix has no dedicated regression test for the partial-save scenario requested in review.

Fix these issues in Kilo Cloud

Previous review (commit 570e193)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • openwisp_controller/pki/base/models.py
  • openwisp_controller/pki/tests/test_models.py

Previous review (commit faf5987)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
openwisp_controller/pki/base/models.py 78 save() now enforces CA-signature verification for every new cert and can raise an uncaught ValidationError on user-supplied certificates not signed by the selected CA (can surface as a 500 in the PKI API)
Files Reviewed (4 files)
  • openwisp_controller/pki/base/models.py - 1 issue
  • openwisp_controller/config/tests/test_template.py - 0 issues
  • openwisp_controller/pki/tests/test_api.py - 0 issues
  • openwisp_controller/pki/tests/test_models.py - 0 issues

Fix these issues in Kilo Cloud

Previous review (commit e74169c)

Status: 3 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 3
WARNING 0
SUGGESTION 0
Issue Details (click to expand)

CRITICAL

File Line Issue
openwisp_controller/config/tests/test_template.py 725 test_clear_is_atomic_when_revocation_fails fails: template removal and cert revocation are not atomic — manage_device_certs() defers the DeviceCertificate cleanup with transaction.on_commit(), so a revocation failure cannot roll back the removal
openwisp_controller/config/tests/test_template.py 1602 test_cert_copies_ca_extensions fails: CA extensions are not copied to generated certificates (get_client_extensions() returns only DEFAULT_CLIENT_EXTENSIONS when no blueprint is set)
openwisp_controller/config/tests/test_template.py 1619 test_cert_context_uses_join fails: get_cert_context() issues 4 queries (2 per-dc.cert FK lookups) instead of 2 because device_certificate_relations is not fetched with select_related("cert")
Files Reviewed (1 file, incremental since 8ce1c6b)
  • openwisp_controller/config/tests/test_template.py - 3 issues

The latest commit 4f4f647 [tests] Added failing tests adds four tests, three of which fail against the current implementation (see above); the PR is not mergeable until the corresponding fixes land so these tests pass. test_no_blueprint_cert_uses_current_ca_key (line 1622) depends on whether Ca.renew() regenerates the CA keypair: _build_cert() keeps the in-memory CA instance, so if renewal rotates the key, that test fails as well.

Fix these issues in Kilo Cloud

Previous review (commit 4f4f647)

Status: 3 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 3
WARNING 0
SUGGESTION 0
Issue Details (click to expand)

CRITICAL

File Line Issue
openwisp_controller/config/tests/test_template.py 725 test_clear_is_atomic_when_revocation_fails fails: template removal and cert revocation are not atomic — manage_device_certs() defers the DeviceCertificate cleanup with transaction.on_commit(), so a revocation failure cannot roll back the removal
openwisp_controller/config/tests/test_template.py 1602 test_cert_copies_ca_extensions fails: CA extensions are not copied to generated certificates (get_client_extensions() returns only DEFAULT_CLIENT_EXTENSIONS when no blueprint is set)
openwisp_controller/config/tests/test_template.py 1619 test_cert_context_uses_join fails: get_cert_context() issues 4 queries (2 per-dc.cert FK lookups) instead of 2 because device_certificate_relations is not fetched with select_related("cert")
Files Reviewed (1 file, incremental since 8ce1c6b)
  • openwisp_controller/config/tests/test_template.py - 3 issues

The latest commit 4f4f647 [tests] Added failing tests adds four tests, three of which fail against the current implementation (see above); the PR is not mergeable until the corresponding fixes land so these tests pass. test_no_blueprint_cert_uses_current_ca_key (line 1622) depends on whether Ca.renew() regenerates the CA keypair: _build_cert() keeps the in-memory CA instance, so if renewal rotates the key, that test fails as well.

Fix these issues in Kilo Cloud

Previous review (commit 8ce1c6b)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file, incremental since 30ff102)
  • docs/user/templates.rst

Previous review (commit 30ff102)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files, incremental since 9d134d6)
  • openwisp_controller/config/tests/test_admin.py
  • openwisp_controller/config/tests/test_device.py
  • openwisp_controller/config/tests/test_template.py
  • openwisp_controller/pki/tests/test_admin.py

Previous review (commit 9d134d6)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files, incremental since c789eca)
  • docs/user/certificate-templates.rst
  • docs/user/settings.rst
  • openwisp_controller/config/tests/test_template.py

Previous review (commit c789eca)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file, incremental since 6727eac)
  • openwisp_controller/pki/tests/test_admin.py

Previous review (commit 6727eac)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
openwisp_controller/pki/tests/test_admin.py 176 resolve(url) is passed a URL that includes the query string, so django.urls.resolve() raises Resolver404 and the two new forged-param changelist regressions error out instead of testing the fix (same defect at line 225)
Files Reviewed (10 files, incremental since 0a1e011)
  • AGENTS.md
  • docs/developer/installation.rst
  • docs/index.rst
  • openwisp_controller/config/tests/test_selenium.py
  • openwisp_controller/config/whois/tests/tests.py
  • openwisp_controller/connection/tests/test_selenium.py
  • openwisp_controller/geo/tests/test_selenium.py
  • openwisp_controller/pki/admin.py
  • openwisp_controller/pki/tests/test_admin.py
  • openwisp_controller/tests/test_selenium.py

The P1 concern raised on pki/admin.py (forged app_label/model_name/field_name params exposing shared CAs and certificates on non-autocomplete admin routes) is addressed in this increment: _source_allows_shared_relation() is now restricted to the admin:autocomplete route, verifies view permission on the source admin, and regression tests were added for forged changelist, change, and delete requests on both Ca and Cert.

Fix these issues in Kilo Cloud

Previous review (commit 0a1e011)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files, incremental since 0b0a252)
  • openwisp_controller/config/base/device_certificate.py
  • openwisp_controller/config/base/template.py
  • openwisp_controller/config/tests/test_device.py
  • openwisp_controller/config/tests/test_template.py

Previous review (commit 0b0a252)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (12 files)
  • docs/user/rest-api.rst
  • docs/user/settings.rst
  • openwisp_controller/config/base/config.py
  • openwisp_controller/config/base/device_certificate.py
  • openwisp_controller/config/base/template.py
  • openwisp_controller/config/static/config/js/switcher.js
  • openwisp_controller/config/tests/test_api.py
  • openwisp_controller/config/tests/test_config.py
  • openwisp_controller/config/tests/test_selenium.py
  • openwisp_controller/config/tests/test_template.py
  • openwisp_controller/pki/api/views.py
  • openwisp_controller/pki/tests/test_api.py

Previous review (commit 881f1f8)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)
  • openwisp_controller/config/base/device_certificate.py
  • openwisp_controller/config/tests/test_selenium.py
  • openwisp_controller/config/tests/test_template.py
  • openwisp_controller/config/tests/test_vpn.py

Previous review (commit 0fb2397)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)
  • openwisp_controller/config/base/device_certificate.py
  • openwisp_controller/pki/base/models.py
  • openwisp_controller/pki/tests/test_admin.py
  • openwisp_controller/pki/tests/test_api.py

Previous review (commit dc53718)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)
  • docs/developer/extending.rst
  • docs/user/certificate-templates.rst
  • docs/user/settings.rst
  • docs/user/templates.rst

Previous review (commit 1f6fe30)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)
  • docs/developer/extending.rst
  • docs/user/certificate-templates.rst
  • docs/user/settings.rst
  • docs/user/templates.rst

Reviewed by balanced · Input: 39.7K · Output: 16.6K · Cached: 877.8K

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
{"name":"HttpError","status":500,"request":{"method":"PATCH","url":"https://api.github.com/repos/openwisp/openwisp-controller/issues/comments/4548211157","headers":{"accept":"application/vnd.github.v3+json","user-agent":"octokit.js/0.0.0-development octokit-core.js/7.0.6 Node.js/24","authorization":"token [REDACTED]","content-type":"application/json; charset=utf-8"},"body":{"body":"<!-- This is an auto-generated comment: summarize by coderabbit.ai -->\n<!-- review_stack_entry_start -->\n\n[![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/openwisp/openwisp-controller/pull/1378?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)\n\n<!-- review_stack_entry_end -->\n<!-- This is an auto-generated comment: review in progress by coderabbit.ai -->\n\n> [!NOTE]\n> Currently processing new changes in this PR. This may take a few minutes, please wait...\n> \n> <details>\n> <summary>⚙️ Run configuration</summary>\n> \n> **Configuration used**: Organization UI\n> \n> **Review profile**: ASSERTIVE\n> \n> **Plan**: Pro\n> \n> **Run ID**: `33bb61f8-c083-446e-8e45-44d753e7ff7b`\n> \n> </details>\n> \n> <details>\n> <summary>📥 Commits</summary>\n> \n> Reviewing files that changed from the base of the PR and between dc55622dfd09741ac51aad38afaaa206714ca875 and 25f1a213225299ecb5dc0ae4960630f68f8d8480.\n> \n> </details>\n> \n> <details>\n> <summary>📒 Files selected for processing (3)</summary>\n> \n> * `openwisp_controller/config/base/template.py`\n> * `openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py`\n> * `tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py`\n> \n> </details>\n> \n> ```ascii\n>  __________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________\n> < I've seen things you people wouldn't believe. Inefficient loops on fire off the shoulder of Orion. I've observed algorithms unfold in the dark near the Tannhäuser Gate, and watched data structures dissolve into the void of garbage collection. All those moments will be lost in my transient GPU cache, like tears in rain. >\n>  ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------\n>   \\\n>    \\   (\\__/)\n>        (•ㅅ•)\n>        /   づ\n> ```\n\n<!-- end of auto-generated comment: review in progress by coderabbit.ai -->\n\n<!-- finishing_touch_checkbox_start -->\n\n<details>\n<summary>✨ Finishing Touches</summary>\n\n<details>\n<summary>🧪 Generate unit tests (beta)</summary>\n\n- [ ] <!-- {\"checkboxId\": \"f47ac10b-58cc-4372-a567-0e02b2c3d479\", \"radioGroupId\": \"utg-output-choice-group-4548221491\"} -->   Create PR with unit tests\n- [ ] <!-- {\"checkboxId\": \"6ba7b810-9dad-11d1-80b4-00c04fd430c8\", \"radioGroupId\": \"utg-output-choice-group-4548221491\"} -->   Commit unit tests in branch `issues/1356-extend-abstract-template`\n\n</details>\n\n</details>\n\n<!-- finishing_touch_checkbox_end -->\n<!-- tips_start -->\n\n---\n\nThanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=openwisp/openwisp-controller&utm_content=1378)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.\n\n<details>\n<summary>❤️ Share</summary>\n\n- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai)\n- [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)\n- [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)\n- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)\n\n</details>\n\n\n<sub>Comment `@coderabbitai help` to get the list of available commands and usage tips.</sub>\n\n<!-- tips_end -->"},"request":{"retryCount":3,"signal":{},"retries":3,"retryAfter":16}}}

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between dc55622 and 25f1a21.

📒 Files selected for processing (3)
  • openwisp_controller/config/base/template.py
  • openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
  • tests/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.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • openwisp_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.py
  • openwisp_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!

Comment thread openwisp_controller/config/base/template.py Outdated
@openwisp-companion

Copy link
Copy Markdown
{
  "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 Analysis

Hello @stktyagi,
(Analysis for commit 25f1a21)

  • Migrations Out of Sync: The CI build failed because the database migrations are not up-to-date. This is indicated by the error message "Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue!".

Fix: Run ./manage.py makemigrations to generate the necessary migration files and then commit them to the repository.

stktyagi added 2 commits May 27, 2026 01:58
Updated previous tests and added new tests for implemetation.

Fixes #1356
Fixed help text for auto cert and updated migration files
@openwisp-companion

Copy link
Copy Markdown

Migrations Check Failed

Hello @stktyagi,
(Analysis for commit a795e09)

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:
Run the following command to apply the pending migrations:

./manage.py makemigrations

@openwisp-companion

Copy link
Copy Markdown

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3).

@stktyagi

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between dc55622 and b946d26.

📒 Files selected for processing (5)
  • openwisp_controller/config/base/template.py
  • openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
  • openwisp_controller/config/tests/test_template.py
  • openwisp_controller/pki/tests/test_api.py
  • tests/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.py
  • tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
  • openwisp_controller/pki/tests/test_api.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
  • openwisp_controller/pki/tests/test_api.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
  • openwisp_controller/pki/tests/test_api.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
  • openwisp_controller/pki/tests/test_api.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • openwisp_controller/pki/tests/test_api.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • openwisp_controller/pki/tests/test_api.py
  • openwisp_controller/config/base/template.py
  • openwisp_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.py
  • tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
  • openwisp_controller/pki/tests/test_api.py
  • openwisp_controller/config/base/template.py
  • openwisp_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

Comment thread openwisp_controller/config/base/template.py Outdated
Comment thread openwisp_controller/config/base/template.py Outdated
@openwisp-companion

Copy link
Copy Markdown

Migrations Check Failed

Hello @stktyagi,
(Analysis for commit b946d26)

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:
Run the following command to generate the missing migrations:

./manage.py makemigrations

stktyagi and others added 2 commits May 27, 2026 09:34
Validate cert relations only inside the cert branch and Only coerce missing cert configs, not every falsy value.

Fixes #1356
Comment thread openwisp_controller/config/base/template.py
Added test for the validation branch that now skips ca / blueprint_cert checks for non-cert templates

Fixes #1356
@coveralls

coveralls commented May 27, 2026

Copy link
Copy Markdown

Coverage Status

Coverage is 97.982%issues/1356-extend-abstract-template into gsoc26-x509-certificate-generator-templates. No base build found for gsoc26-x509-certificate-generator-templates.

Fixed line too long flake error

Fixes #1356
…1377

Implemented the DeviceCertificate M2M through-model to act as a strict relational bridge between Config, Template, and django_x509.Cert.

Fixes #1377
@stktyagi stktyagi added the gsoc Part of a Google Summer of Code project label May 28, 2026
Updated test by joining the list of strings into one sentence.

Fixes #1377
@stktyagi stktyagi changed the title [feature] Extended Template model for standalone X.509 certificates #1356 [feature] Introduced standalone certificate templates and device bindings May 29, 2026
stktyagi and others added 3 commits September 1, 2026 21:59
Fixed both reviews with P2 priority
Fixed atomicity issue in post_clear
@openwisp openwisp deleted a comment from openwisp-companion Bot Sep 1, 2026
@openwisp openwisp deleted a comment from openwisp-companion Bot Sep 1, 2026
@openwisp openwisp deleted a comment from openwisp-companion Bot Sep 1, 2026
@openwisp openwisp deleted a comment from openwisp-companion Bot Sep 1, 2026
@openwisp openwisp deleted a comment from openwisp-companion Bot Sep 1, 2026
Serialized certificate minting with ca renewal
Comment thread openwisp_controller/pki/base/models.py
Fixed the uncaught validation error in save.
@openwisp openwisp deleted a comment from openwisp-companion Bot Sep 1, 2026
@openwisp-companion

Copy link
Copy Markdown

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3).

@nemesifier

Copy link
Copy Markdown
Member

@coderabbitai is there any outstanding issue?

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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.html

Length of output: 40735


@nemesifier No confirmed functional issue remains in #1378.

  • The PR is merge-clean.
  • All completed CI checks pass.
  • The certificate lifecycle and OID-extension concerns were previously verified and withdrawn.
  • The requested documentation, certificate context variables, and device-admin certificate details are present.
  • TemplateSerializer includes writable ca and blueprint_cert fields.

One non-blocking verification remains. I did not render /api/v1/docs/ or the DRF browsable API. The serializer evidence indicates that both fields are available, but a UI-level check would confirm their presentation.

GitHub still shows CHANGES_REQUESTED because unresolved review threads remain. Those threads do not indicate a confirmed defect in the current branch.


🧠 Learnings used
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/api/serializers.py:297-300
Timestamp: 2026-05-31T10:40:20.981Z
Learning: In openwisp/openwisp-controller, `AbstractConfig.manage_device_certs` (config/base/config.py) intentionally ignores `post_clear` for active devices to prevent certificate revocation during sortedm2m template reordering. Because `Config.templates.set(..., clear=True)` fires `post_clear` (not `post_remove`), the orphan-cleanup logic in the `post_remove`/`post_add` branch is never triggered during API PUT updates. The explicit `DeviceCertificate.objects.filter(config=config).exclude(template_id__in=config_templates).delete()` call in `DeviceConfigSerializer._update_config()` (config/api/serializers.py) is therefore intentional and necessary to revoke and clean up certificates for removed cert templates before calling `config.templates.set(config_templates, clear=True)`.

You are interacting with an AI system.

Comment thread docs/user/certificate-templates.rst Outdated
Comment on lines +280 to +283
- **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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread docs/user/settings.rst
Comment on lines +347 to +350
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The renewal request reads the old certificate while revoked=False.
  2. This task revokes it and points DeviceCertificate to the replacement.
  3. The renewal save resumes and writes its stale revoked=False value 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">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Comment thread openwisp_controller/config/base/device.py
@stktyagi

stktyagi commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai check the latest commit (06a0528)

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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' || true

Length 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}' \
  || true

Length of output: 4057


@stktyagi I checked commit 06a0528.

The certificate locking changes are consistent with the inspected django-x509 1.4 renew() and revoke() implementations. I found no confirmed source-level defect in the changed code.

CI currently has a blocking regression:

  • The parent commit passed all Django 4.2 jobs.
  • Commit 06a0528 fails all three Django 4.2 jobs.
  • Each failed job reports failures in both ./run-qa-checks and ./runtests.
  • The workflow is still in progress, so GitHub does not expose the failed-step logs yet.

The failure cause is inconclusive until the workflow completes. Please retry the check after completion so I can inspect the job output.


🧠 Learnings used
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_template.py:641-702
Timestamp: 2026-08-30T10:44:07.306Z
Learning: In `openwisp_controller/config/tests/test_template.py`, `test_concurrent_assignment_and_cert_template_mutation` belongs to `TestTemplateTransaction`, which inherits from Django `TransactionTestCase`. Its event ordering intentionally tests the race where `Template.full_clean()` completes before certificate-template assignment and `Template.save(update_fields=["ca"])` must re-check protected fields after assignment.

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/base/device_certificate.py:82-85
Timestamp: 2026-08-12T23:29:17.967Z
Learning: In `openwisp_controller/config/base/device_certificate.py`, `AbstractDeviceCertificate.clean()` must guard every access to `self.config.device` with `self.config_id`. Django `Model.full_clean()` invokes `clean()` even if `clean_fields()` found a missing required `config`, so an incomplete `DeviceCertificate` must raise `ValidationError` instead of `RelatedObjectDoesNotExist`.

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/base/template.py:285-294
Timestamp: 2026-08-12T23:29:06.019Z
Learning: In OpenWISP Controller, `DeviceCertificate` is swappable. Do not use class-name-derived reverse accessors such as `devicecertificate_set` or `devicecertificate__` in `openwisp_controller/config/base/config.py`, `openwisp_controller/config/base/template.py`, or `openwisp_controller/config/x509_admin.py`. Define stable `related_name` values and provide coverage for a custom swapped `DeviceCertificate` concrete class.

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/api/serializers.py:362-362
Timestamp: 2026-08-12T23:29:03.745Z
Learning: In `openwisp_controller/config/api/serializers.py`, `DeviceDetailSerializer.update` must delete related `DeviceCertificate` rows when a configured device changes organization. Generated certificates are owned by the device organization, so retaining a certificate for a shared required certificate template would incorrectly retain certificate ownership from the previous organization. The deletion revokes the old certificate and template reconciliation provisions a certificate for the new organization.

Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/api/serializers.py:297-300
Timestamp: 2026-05-31T10:40:20.981Z
Learning: In openwisp/openwisp-controller, `AbstractConfig.manage_device_certs` (config/base/config.py) intentionally ignores `post_clear` for active devices to prevent certificate revocation during sortedm2m template reordering. Because `Config.templates.set(..., clear=True)` fires `post_clear` (not `post_remove`), the orphan-cleanup logic in the `post_remove`/`post_add` branch is never triggered during API PUT updates. The explicit `DeviceCertificate.objects.filter(config=config).exclude(template_id__in=config_templates).delete()` call in `DeviceConfigSerializer._update_config()` (config/api/serializers.py) is therefore intentional and necessary to revoke and clean up certificates for removed cert templates before calling `config.templates.set(config_templates, clear=True)`.

You are interacting with an AI system.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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
    done

Length 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
done

Length 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' || true

Length 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
done

Length of output: 29578


@stktyagi I checked commit 06a0528. I found no confirmed functional issue in the changed code.

  • The certificate row locks are correct for the stale-renewal race. django-x509 1.4 calls save() and emits x509_renewed during renewal. The new implementation keeps these side effects.
  • The management_ip change check now correctly ignores dirty in-memory values when update_fields excludes management_ip.
  • The deleted help.svg file has no remaining repository references.
  • The diff has no whitespace errors.

CI has a separate blocker. All Django 4.2 jobs fail before the test suite runs. Dependency resolution installs django-filter 25.2 and django-redis 7.0.0, which require Django 5.2 or later. Django then fails system checks for openwisp_notifications.Notification.type and NotificationSetting.type. This failure is not caused by the files changed in 06a0528.

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 used
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_template.py:641-702
Timestamp: 2026-08-30T10:44:07.306Z
Learning: In `openwisp_controller/config/tests/test_template.py`, `test_concurrent_assignment_and_cert_template_mutation` belongs to `TestTemplateTransaction`, which inherits from Django `TransactionTestCase`. Its event ordering intentionally tests the race where `Template.full_clean()` completes before certificate-template assignment and `Template.save(update_fields=["ca"])` must re-check protected fields after assignment.

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/base/device_certificate.py:82-85
Timestamp: 2026-08-12T23:29:17.967Z
Learning: In `openwisp_controller/config/base/device_certificate.py`, `AbstractDeviceCertificate.clean()` must guard every access to `self.config.device` with `self.config_id`. Django `Model.full_clean()` invokes `clean()` even if `clean_fields()` found a missing required `config`, so an incomplete `DeviceCertificate` must raise `ValidationError` instead of `RelatedObjectDoesNotExist`.

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/base/template.py:285-294
Timestamp: 2026-08-12T23:29:06.019Z
Learning: In OpenWISP Controller, `DeviceCertificate` is swappable. Do not use class-name-derived reverse accessors such as `devicecertificate_set` or `devicecertificate__` in `openwisp_controller/config/base/config.py`, `openwisp_controller/config/base/template.py`, or `openwisp_controller/config/x509_admin.py`. Define stable `related_name` values and provide coverage for a custom swapped `DeviceCertificate` concrete class.

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/api/serializers.py:362-362
Timestamp: 2026-08-12T23:29:03.745Z
Learning: In `openwisp_controller/config/api/serializers.py`, `DeviceDetailSerializer.update` must delete related `DeviceCertificate` rows when a configured device changes organization. Generated certificates are owned by the device organization, so retaining a certificate for a shared required certificate template would incorrectly retain certificate ownership from the previous organization. The deletion revokes the old certificate and template reconciliation provisions a certificate for the new organization.

Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/api/serializers.py:297-300
Timestamp: 2026-05-31T10:40:20.981Z
Learning: In openwisp/openwisp-controller, `AbstractConfig.manage_device_certs` (config/base/config.py) intentionally ignores `post_clear` for active devices to prevent certificate revocation during sortedm2m template reordering. Because `Config.templates.set(..., clear=True)` fires `post_clear` (not `post_remove`), the orphan-cleanup logic in the `post_remove`/`post_add` branch is never triggered during API PUT updates. The explicit `DeviceCertificate.objects.filter(config=config).exclude(template_id__in=config_templates).delete()` call in `DeviceConfigSerializer._update_config()` (config/api/serializers.py) is therefore intentional and necessary to revoke and clean up certificates for removed cert templates before calling `config.templates.set(config_templates, clear=True)`.

You are interacting with an AI system.

@openwisp-companion

Copy link
Copy Markdown

Django System Check Error: Choices Must Be an Iterable

Hello @stktyagi,
(Analysis for commit 06a0528)

  • Test Failure / System Check Error: Django is throwing fields.E004 because the choices argument on openwisp_notifications.Notification.type and openwisp_notifications.NotificationSetting.type is not an iterable (such as a list or tuple).
  • Fix: Ensure that the choices parameter defined on the type field for both models is properly set to a list or tuple of two-tuples (e.g., CHOICES = (('value', 'Label'),)). Review the model definitions in openwisp_notifications where these fields are declared and correct the non-iterable definition.

@openwisp-companion

Copy link
Copy Markdown

Django System Check Choice Field Errors

Hello @stktyagi,
(Analysis for commit 06a0528)

  • Build/Infrastructure/Other (Django System Check Error): The Django system check is failing because the choices argument on openwisp_notifications.Notification.type and openwisp_notifications.NotificationSetting.type is not an iterable (such as a list or tuple).
  • Fix: Ensure that the choices parameter assigned to the type field in both Notification and NotificationSetting models is correctly defined as a list or tuple of 2-tuples (e.g., choices=[('value', 'Label'), ...]).

@openwisp-companion

Copy link
Copy Markdown

Django system check errors on model choices

Hello @stktyagi,
(Analysis for commit 06a0528)

  • Build/Infrastructure/Other: The Django system check failed because the choices argument on openwisp_notifications.Notification.type and openwisp_notifications.NotificationSetting.type must be an iterable (such as a list or tuple) rather than a non-iterable or incorrectly configured value.
  • Fix: Ensure that the choices definitions for Notification.type and NotificationSetting.type are properly defined as a list or tuple of 2-tuples in your model fields or swappable settings.

@openwisp-companion

Copy link
Copy Markdown

Django System Check Error: Choices Must Be Iterable

Hello @stktyagi,
(Analysis for commit 06a0528)

  • Test Failure / Build Error: The Django system check failed because the choices argument on Notification.type and NotificationSetting.type fields in openwisp-notifications must be an iterable (such as a list or a tuple), but it was passed an invalid type (likely None or a non-iterable object due to configuration or version incompatibility).
  • Fix: Ensure that the notification type choices are properly defined as a list or tuple of two-tuples where they are configured or overridden.

stktyagi and others added 2 commits September 3, 2026 07:42
@openwisp-companion

Copy link
Copy Markdown

Fix unused import in test_admin.py

Hello @stktyagi,
(Analysis for commit b72eebb)

  • Code Style/QA (flake8 failure):
  • Error: ./openwisp_controller/config/tests/test_admin.py:5:1: F401 'types.SimpleNamespace' imported but unused
  • Fix: Remove the unused import types.SimpleNamespace from line 5 of openwisp_controller/config/tests/test_admin.py.

@openwisp-companion

Copy link
Copy Markdown

Transient Infrastructure Failure

Hello @stktyagi,
(Analysis for commit a5099f8)

This looks like a transient infrastructure issue (Coveralls / test runner coverage warning: CoverageWarning: No data was collected. (no-data-collected)). The CI has been restarted automatically.

mocked_task.assert_called_once()


class TestDeviceCertificateRegenerationTaskTransaction(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement gsoc Part of a Google Summer of Code project

Projects

Development

Successfully merging this pull request may close these issues.

4 participants