[feature] Introduced standalone certificate templates and device bindings#1378
[feature] Introduced standalone certificate templates and device bindings#1378stktyagi wants to merge 42 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR introduces a complete X.509 certificate template system for OpenWISP. It extends Sequence Diagram(s)sequenceDiagram
participant Admin/API
participant AbstractTemplate
participant AbstractConfig
participant manage_device_certs
participant AbstractDeviceCertificate
participant X509Cert
Admin/API->>AbstractTemplate: save cert template with ca
AbstractTemplate->>AbstractTemplate: clean() validates ca & blueprint_cert
Admin/API->>AbstractConfig: attach to device config
AbstractConfig->>manage_device_certs: m2m_changed post_add
manage_device_certs->>AbstractDeviceCertificate: create(config, template, auto_cert=True)
AbstractDeviceCertificate->>AbstractDeviceCertificate: save() calls _auto_x509()
AbstractDeviceCertificate->>X509Cert: create client cert with device CN & OIDs
X509Cert->>AbstractDeviceCertificate: assign to cert field
AbstractDeviceCertificate->>AbstractConfig: device context now includes cert PEM/key
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Review (since a819721)The new commit
No new critical bugs or security vulnerabilities were introduced by this commit. Files Reviewed (incremental)
Carried-forward from prior reviewAll previously reviewed files and their findings carry forward unchanged. The prior outstanding note (missing regression test for cert-template validation in Previous Review Summaries (2 snapshots, latest commit a819721)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a819721)Status: No Issues Found | Recommendation: Merge Incremental Review (since 298572f)The new commits add a large body of work: certificate regeneration on hardware drift (#1359), deactivated-device enforcement across config/connection/WHOIS/geo, pagination consolidation to The previously flagged WARNING (missing regression test for cert-template validation in Review highlights:
Extensive new tests accompany each change and Files Reviewed (incremental)
Previous review (commit 298572f)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Previous findings on No additional issues in unchanged code. Files Reviewed (5 files)
Reviewed by glm-5.2-20260616 · 696,712 tokens |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/config/base/template.py`:
- Around line 265-267: The help text for the auto_cert field is out of date (it
still says it's only valid for VPN templates) — update the auto_cert field's
help/verbose/help_text in the Template definition in
openwisp_controller/config/base/template.py so it matches the new behavior
(auto_cert is allowed when type == "cert" as well as when type == "vpn"); locate
the auto_cert attribute (and any admin/API serializer or form label/help_text
referencing it) and change the message to something like "Valid for 'vpn' and
'cert' template types" or equivalent clear wording that includes both types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33bb61f8-c083-446e-8e45-44d753e7ff7b
📒 Files selected for processing (3)
openwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
🧠 Learnings (4)
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (3)
openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 62-83, 251-253, 271-312
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
1-59: LGTM!tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
1-58: LGTM!
{
"failures": [
{
"type": "Build/Infrastructure/Other",
"message": "Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue!",
"details": "The CI environment detected that the database migrations are out of sync. This is indicated by the error message 'Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue!' and the mention of a specific migration file '0005_user_expiration_date_user_user_active_expiry_idx.py'."
}
]
}CI Failure AnalysisHello @stktyagi,
Fix: Run |
Updated previous tests and added new tests for implemetation. Fixes #1356
Fixed help text for auto cert and updated migration files
Migrations Check FailedHello @stktyagi, The CI failed because there are unapplied database migrations. This means that the database schema is out of sync with the current state of the code. Fix: ./manage.py makemigrations |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/config/base/template.py`:
- Around line 252-253: The validation of certificate relations (calls to
self._validate_org_relation("ca") and
self._validate_org_relation("blueprint_cert")) is running unconditionally and
can fail on stale relations when the object is being switched away from the
"cert" type; restrict these validations to the cert branch so they only run when
the template's type is "cert" (e.g., wrap or move the
self._validate_org_relation(...) calls inside the same conditional that handles
the "cert" branch or after the type check that preserves/clears ca and
blueprint_cert), ensuring they do not run when the code path clears those fields
(see the branch that clears ca and blueprint_cert).
- Around line 304-305: The current check "if not self.config" coerces any falsy
value ([], "", False) into {}, bypassing BaseConfig.clean() validation; change
the condition to only handle missing configs by checking "if self.config is
None" (or equivalent explicit None check) so only absent configs are replaced
with {} and invalid/falsy payloads are left intact for
full_clean()/BaseConfig.clean() to reject.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3d93328-e58d-41dd-a374-dffebd6d1e38
📒 Files selected for processing (5)
openwisp_controller/config/base/template.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
🧠 Learnings (4)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_template.py
📚 Learning: 2026-01-12T22:27:40.078Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:40.078Z
Learning: In test migrations under tests/openwisp2/sample_config/migrations, verify scenarios where a swappable model (CONFIG_WHOISINFO_MODEL) is extended with extra fields (e.g., an additional 'details' field) to ensure compatibility and no errors when swapping to a custom implementation. This pattern helps confirm that extending AbstractWHOISInfo via a custom model works as intended.
Applied to files:
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
🔇 Additional comments (4)
openwisp_controller/pki/tests/test_api.py (1)
155-155: LGTM!Also applies to: 275-275
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
12-16: LGTM!Also applies to: 19-44, 45-58, 59-74
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
11-14: LGTM!Also applies to: 17-42, 43-56, 57-72
openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 62-83, 119-120
Migrations Check FailedHello @stktyagi, The CI failed because there are unapplied database migrations. Failure: Migrations check failed! Models' changes not migrated, please run './manage.py makemigrations' to solve the issue! Fix: ./manage.py makemigrations |
…s/1356-extend-abstract-template
Validate cert relations only inside the cert branch and Only coerce missing cert configs, not every falsy value. Fixes #1356
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Added test for the validation branch that now skips ca / blueprint_cert checks for non-cert templates Fixes #1356
|
Coverage is 98.385% — issues/1356-extend-abstract-template into gsoc26-x509-certificate-generator-templates. No base build found for |
Fixed line too long flake error Fixes #1356
Updated test by joining the list of strings into one sentence. Fixes #1377
stktyagi
changed the title
Made task safe against overlapping runs for the same device.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
Updated certificate_updated to synchronize config status
Addressed review comments for updating documentation
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I left a few inline comments on the remaining issues.
| certs_regenerated = 0 | ||
|
|
||
| with transaction.atomic(): | ||
| active_device_certs = ( |
There was a problem hiding this comment.
The row lock prevents two workers from modifying the same DeviceCertificate at the same time, but the task is still not idempotent. If two hardware-drift tasks are queued for the same device, the second worker waits for the first one to commit, then this queryset matches the newly generated non-revoked certificate and rotates it again. A device may fetch the config generated by the first task and then have that certificate revoked immediately by the duplicate task.
I would add an event guard before revoking the current cert, for example pass the expected old cert IDs or expected old hardware values to the task and skip rows that no longer match that event. A regression test should queue or run the task twice for the same hardware change and verify only one replacement cert is generated.
| }, | ||
| ] | ||
| ) | ||
| new_cert = Cert( |
There was a problem hiding this comment.
The regeneration path manually creates the replacement Cert, while initial provisioning goes through DeviceCertificate._auto_create_cert_extra(). This means a project using a swappable DeviceCertificate model to customize certificate creation will get different behavior during hardware-drift regeneration than during initial provisioning.
I would call dc._auto_create_cert_extra(new_cert) before full_clean() here, or ensure the logic is defined in a model and not duplicated.
There was a problem hiding this comment.
@stktyagi - lets refactor this duplication:
pull the build logic up into the model:
# in AbstractDeviceCertificate
def _build_cert(self, name, common_name):
"""Build (but do not save) a Cert instance from template + blueprint."""
ca = self.template.ca
blueprint = self.template.blueprint_cert
cert_model = self.__class__.cert.field.related_model
attrs = self._clone_blueprint_attrs(ca, blueprint)
extensions = self._build_extensions(blueprint)
cert = cert_model(
name=name, ca=ca, common_name=common_name,
extensions=extensions, **attrs,
)
return self._auto_create_cert_extra(cert)
def _clone_blueprint_attrs(self, ca, blueprint):
source = blueprint or ca
digest = str(source.digest) if not blueprint else source.digest
return dict(
key_length=source.key_length,
digest=digest,
country_code=source.country_code,
state=source.state,
city=source.city,
organization_name=source.organization_name,
email=source.email,
)
def _build_extensions(self, blueprint):
if blueprint and blueprint.extensions:
extensions = copy.deepcopy(blueprint.extensions)
else:
extensions = [{"name": "nsCertType", "value": "client", "critical": False}]
extensions.extend(self._get_hardware_oid_extensions())
return extensions
def _get_hardware_oid_extensions(self):
device = self.config.device
return [
{"oid": MAC_ADDRESS_OID, "value": f"ASN1:UTF8:string:{device.mac_address}", "critical": False},
{"oid": DEVICE_UUID_OID, "value": f"ASN1:UTF8:string:{device.id}", "critical": False},
]Then _auto_create_cert() and the regen task both collapse to:
cert = dc._build_cert(name=device.name, common_name=dc._get_common_name())
cert.full_clean()
cert.save()
dc.cert = cert
dc.save()| using the template's 32-character hexadecimal UUID (the UUID with dashes | ||
| removed). | ||
|
|
||
| The following variables are dynamically injected into the configuration |
There was a problem hiding this comment.
The certificate context section says "the following variables" but omits cert_<template_uuid_hex>_path and cert_<template_uuid_hex>_key_path, which are exposed by get_cert_context() and tested. Please add these two public variables here.
| CONTEXT = get_setting("CONTEXT", {}) | ||
| assert isinstance(CONTEXT, dict), "OPENWISP_CONTROLLER_CONTEXT must be a dictionary" | ||
| DEFAULT_AUTO_CERT = get_setting("DEFAULT_AUTO_CERT", True) | ||
| REGENERATE_CERTS_ON_HARDWARE_CHANGE = get_setting( |
There was a problem hiding this comment.
This adds the public OPENWISP_CONTROLLER_REGENERATE_CERTS_ON_HARDWARE_CHANGE setting, but I do not see it documented in docs/user/settings.rst. While updating that page, please also update OPENWISP_CONTROLLER_COMMON_NAME_FORMAT, which still says it only defines VPN client certificate common names even though standalone certificate templates also use it.
Addressed review feedback to ensure the hardware drift certificate regeneration lifecycle is secure, extensible, and idempotent
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Aryamanz29
left a comment
Aryamanz29
left a comment
There was a problem hiding this comment.
good progress - few minor suggestions:
| }, | ||
| ] | ||
| ) | ||
| new_cert = Cert( |
There was a problem hiding this comment.
@stktyagi - lets refactor this duplication:
pull the build logic up into the model:
# in AbstractDeviceCertificate
def _build_cert(self, name, common_name):
"""Build (but do not save) a Cert instance from template + blueprint."""
ca = self.template.ca
blueprint = self.template.blueprint_cert
cert_model = self.__class__.cert.field.related_model
attrs = self._clone_blueprint_attrs(ca, blueprint)
extensions = self._build_extensions(blueprint)
cert = cert_model(
name=name, ca=ca, common_name=common_name,
extensions=extensions, **attrs,
)
return self._auto_create_cert_extra(cert)
def _clone_blueprint_attrs(self, ca, blueprint):
source = blueprint or ca
digest = str(source.digest) if not blueprint else source.digest
return dict(
key_length=source.key_length,
digest=digest,
country_code=source.country_code,
state=source.state,
city=source.city,
organization_name=source.organization_name,
email=source.email,
)
def _build_extensions(self, blueprint):
if blueprint and blueprint.extensions:
extensions = copy.deepcopy(blueprint.extensions)
else:
extensions = [{"name": "nsCertType", "value": "client", "critical": False}]
extensions.extend(self._get_hardware_oid_extensions())
return extensions
def _get_hardware_oid_extensions(self):
device = self.config.device
return [
{"oid": MAC_ADDRESS_OID, "value": f"ASN1:UTF8:string:{device.mac_address}", "critical": False},
{"oid": DEVICE_UUID_OID, "value": f"ASN1:UTF8:string:{device.id}", "critical": False},
]Then _auto_create_cert() and the regen task both collapse to:
cert = dc._build_cert(name=device.name, common_name=dc._get_common_name())
cert.full_clean()
cert.save()
dc.cert = cert
dc.save()|
|
||
| # inject MAC and UUID as custom OIDs, prerequisite: #228 in django-x509 | ||
| mac_oid = "1.3.6.1.4.1.65901.1" | ||
| uuid_oid = "1.3.6.1.4.1.65901.2" |
There was a problem hiding this comment.
lets make these named constants:
Side benefit: gives future maintainers a grep-able name and a place to add a comment about which OID arc this is
# device_certificate.py (top of file)
MAC_ADDRESS_OID = "1.3.6.1.4.1.65901.1"
DEVICE_UUID_OID = "1.3.6.1.4.1.65901.2"| if update_fields is not None: | ||
| if "name" not in update_fields and "mac_address" not in update_fields: | ||
| return |
There was a problem hiding this comment.
refactor:
def _hardware_fields_changed(update_fields):
return update_fields is None or ("name" in update_fields or "mac_address" in update_fields)
| if update_fields is not None: | ||
| if "name" not in update_fields and "mac_address" not in update_fields: | ||
| return |
There was a problem hiding this comment.
then use _hardware_fields_changed()
…s/1356-extend-abstract-template
Added selenium tests and refactored DeviceCertificate model
|
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 |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/config/admin.py`:
- Around line 118-125: The js list in BaseConfigAdmin.Media includes a reference
to template_ui.js which does not exist in the static files directory and will
cause 404 errors for all admin classes inheriting from BaseConfigAdmin. Either
remove the template_ui.js entry from the js list in the Media class if it was
added incorrectly, or if this file should exist, create it in the appropriate
static directory. Alternatively, if the intent was to reference a different
existing file such as relevant_templates.js, update the string reference in the
js list to point to the correct file name.
In `@openwisp_controller/config/api/serializers.py`:
- Around line 440-445: The code path that changes the organization and calls
instance.config.templates.clear() directly bypasses the _update_config() method,
which means the explicit DeviceCertificate deletion logic from Lines 324-326
does not execute. This allows stale device-certificate rows to persist after the
organization change. After calling instance.config.templates.clear() and before
or after Config.enforce_required_templates(), explicitly delete the associated
DeviceCertificate objects using the same deletion logic that exists in
_update_config() to ensure device certificates are properly cleaned up when
templates are cleared during an organization change.
In `@openwisp_controller/config/handlers.py`:
- Around line 48-51: The `_hardware_fields_changed` function correctly
identifies when hardware fields should be checked, but the
`detect_hardware_drift` handler still compares all hardware fields (name,
mac_address) unconditionally against the instance, treating in-memory unsaved
changes as drift. Modify the comparison logic in the `detect_hardware_drift`
handler to only compare the specific fields that were actually saved by checking
if they exist in the `update_fields` parameter when it is not None; only compare
all hardware fields when `update_fields` is None. Additionally, add a regression
test case that sets `mac_address` to a dirty value in memory, calls
`save(update_fields={"name"})` without modifying name, and verifies that the
regeneration task is not queued since mac_address was not persisted.
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 699-705: The test
`test_hardware_drift_signal_ignores_unrelated_changes` does not properly
validate that the hardware drift task is not enqueued. The test lacks an
eligible DeviceCertificate instance and does not trigger on_commit callbacks, so
mocked_task.assert_not_called() passes regardless of regression in the enqueue
logic. Create a DeviceCertificate for the device before saving it, then ensure
on_commit callbacks are executed (using a context manager like
transaction.atomic() or a test utility method that handles database commits),
and verify that the task was genuinely not called because the change (the key
modification) is unrelated to hardware drift, not because the prerequisites were
missing.
In `@openwisp_controller/config/tests/test_selenium.py`:
- Line 25: The code is directly loading the Ca model using
load_model("django_x509", "Ca") which bypasses the existing validation in the
_create_ca() helper method. Replace all direct Ca fixture creation (at line 25
and also around lines 498-500 and 529-531) with calls to the _create_ca() helper
method instead. After replacing all direct Ca instantiations with the helper,
remove the direct Ca model binding from load_model if it is no longer referenced
elsewhere in the test file.
In `@openwisp_controller/config/tests/test_template.py`:
- Around line 1441-1460: The test_get_unassigned_certs_with_null_device_cert
test may not actually create a row with cert_id=NULL in the database because
DeviceCertificate.save() can auto-provision a certificate when auto_cert is true
and cert is missing. Add an explicit assertion immediately after creating the
DeviceCertificate object with cert=None to verify that the cert_id field in the
database is actually NULL before calling get_unassigned_certs(). This ensures
the regression test truly exercises SQL NULL semantics as intended.
🪄 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: 37e188c0-6bf4-4395-b0a7-deb6440e9313
📒 Files selected for processing (29)
docs/developer/extending.rstdocs/index.rstdocs/user/certificate-templates.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/settings.rstdocs/user/templates.rstopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/settings.pyopenwisp_controller/config/static/config/js/switcher.jsopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pytests/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/settings.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pyopenwisp_controller/config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/static/config/js/switcher.jsopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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/settings.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pyopenwisp_controller/config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/static/config/js/switcher.jsopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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/settings.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pyopenwisp_controller/config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/static/config/js/switcher.jsopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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/settings.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pyopenwisp_controller/config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pyopenwisp_controller/config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.py
🧠 Learnings (7)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/settings.pyopenwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pyopenwisp_controller/config/models.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/base/template.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/config/tests/test_admin.pytests/openwisp2/settings.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_config.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/tests/test_device.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/tests/test_api.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
🪛 ast-grep (0.43.0)
tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py
[info] 106-106: use help_text to document model columns
Context: models.CharField(blank=True, max_length=64, null=True)
Note: Security best practice.
(model-help-text)
[warning] 64-75: always specify max_length for a Charfield
Context: models.CharField(
choices=[
("generic", "Generic"),
("vpn", "VPN-client"),
("cert", "Certificate"),
],
db_index=True,
default="generic",
help_text="template type, determines which features are available",
max_length=16,
verbose_name="type",
)
Note: Security best practice.
(model-charfield-max-length)
[warning] 106-106: always specify max_length for a Charfield
Context: models.CharField(blank=True, max_length=64, null=True)
Note: Security best practice.
(model-charfield-max-length)
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py
[warning] 66-77: always specify max_length for a Charfield
Context: models.CharField(
choices=[
("generic", "Generic"),
("vpn", "VPN-client"),
("cert", "Certificate"),
],
db_index=True,
default="generic",
help_text="template type, determines which features are available",
max_length=16,
verbose_name="type",
)
Note: Security best practice.
(model-charfield-max-length)
🔇 Additional comments (21)
openwisp_controller/config/tests/test_device.py (1)
8-8: LGTM!Also applies to: 29-31, 660-694, 721-878
openwisp_controller/config/tests/test_admin.py (1)
2335-2335: LGTM!openwisp_controller/config/tests/test_config.py (1)
864-865: LGTM!Also applies to: 867-870
openwisp_controller/pki/tests/test_api.py (1)
155-155: LGTM!Also applies to: 256-257, 267-276, 292-293
openwisp_controller/config/base/config.py (1)
66-72: LGTM!Also applies to: 475-489, 502-547, 978-1003, 1031-1032
openwisp_controller/config/apps.py (1)
72-72: LGTM!Also applies to: 98-102, 124-128
openwisp_controller/config/settings.py (1)
37-39: LGTM!openwisp_controller/config/handlers.py (1)
2-10: LGTM!Also applies to: 54-69, 72-80, 85-100
openwisp_controller/config/tasks.py (1)
8-15: LGTM!Also applies to: 226-294
openwisp_controller/config/tests/test_api.py (1)
1484-1503: Still coverblueprint_certorganization scoping in the API test.Line 1484 only exercises a cross-organization
ca. The previous API coverage request also called outblueprint_cert; add an isolated case using a valid/shared CA with a blueprint certificate outside the template organization so that RBAC/org scoping for blueprint material cannot regress.openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 40-49, 74-97, 126-135, 256-300, 302-346, 348-384
openwisp_controller/config/base/device_certificate.py (2)
1-55: LGTM!Also applies to: 65-175
57-63: No partial-save calls to DeviceCertificate found—issue is theoretically valid but unmanifested in current codebase.An examination of the codebase reveals no calls to
DeviceCertificate.save(update_fields=...). The single existingDeviceCertificate.save()call atopenwisp_controller/config/tasks.py:269is a full save withoutupdate_fields. While the concern is theoretically sound—ifsave(update_fields=...)excludescert, the newly assignedcert_idfrom_auto_x509()would not persist—no actual code path currently triggers this scenario.The proposed fix is valid defensive programming and would prevent future regressions if partial saves are introduced. Consider implementing it if this model is expected to be subclassed or extended by external code, or if partial-save patterns are planned.
openwisp_controller/config/models.py (1)
5-5: LGTM!Also applies to: 97-105
tests/openwisp2/sample_config/models.py (1)
5-5: LGTM!Also applies to: 99-105
openwisp_controller/config/api/serializers.py (1)
20-85: LGTM!Also applies to: 87-172, 324-327
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
23-152: LGTM!tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
21-150: LGTM!tests/openwisp2/settings.py (1)
284-284: LGTM!openwisp_controller/config/admin.py (1)
1068-1104: LGTM!openwisp_controller/config/static/config/js/switcher.js (1)
4-67: LGTM!
Addressed coderabbit review comments.
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
4 participants
Checklist
Reference to Existing Issue
Closes #1356
Closes #1377
Closes #1357
Closes #1361
Closes #1358
Closes #1360
Closes #1359
Description of Changes
This PR establishes the database architecture, UI, API and lifecycle for standalone X.509 certificate templates.
Manual test plan
Setup
PKI->Certification Authoritiesand create two CAs:CA-1andCA-2.PKI->Certificatesand create two certificates to act as blueprints:Blueprint-1(Must useCA-1)Blueprint-2(Must useCA-2)Devicesand create a device (test-device).Template Creation and Validation
Configuration->Templatesand clickADD TEMPLATE.Certificate.CA-1.Blueprint-2(which belongs toCA-2). Try to save.Blueprint-1. Name the templateActive-Cert-Template. Save it.Device Provisioning
test-device.Active-Cert-Template. Save.PKI->Certificates.test-device. Its status should be valid (not revoked).Active Mutation Locks
Configuration->Templatesand editActive-Cert-Template(which is now assigned to an active device).Generic. Try to save.CA-2. Try to save.Blueprint-2(ensure you also change the CA so they match, triggering the active lock). Try to save.Revocation on Removal
test-device.Active-Cert-Templateentirely from the templates list. Save.PKI->Certificatesand locate the device's certificate.Context Configuration Injection
Go to
Configuration->Templates, openActive-Cert-Template, and copy its UUID from the URL bar (removing the dashes so it is a 32-character hex string).In the JSON configuration editor for the template, add a configuration block that references the certificate's UUID variables:
(Note: Replace
<uuid>with the actual 32-character hex string of the template).Click Save.
Go back to the Configuration page for
test-device(which has this template assigned) and click thePreview configurationbutton.Expected Result: The variables should be successfully resolved. In the preview, you should see the generated path (e.g.,
/etc/x509/cert-<uuid>.pem) and the literal-----BEGIN CERTIFICATE-----text instead of the raw{{ }}template tags.output.mp4