[feature] Introduced standalone certificate templates and device bindings#1378
[feature] Introduced standalone certificate templates and device bindings#1378stktyagi wants to merge 27 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a "cert" template type with CA and blueprint_cert relations, introduces an abstract DeviceCertificate model and a concrete swappable DeviceCertificate, wires AbstractConfig.device_certificates M2M with an m2m_changed handler, implements certificate auto-provisioning and revocation, updates admin/serializer/API surfaces and JS, provides production and sample migrations, and adds unit and Selenium tests covering validation, lifecycle, and regressions. Sequence Diagram(s)sequenceDiagram
participant Client as Client/API
participant Config as ConfigModel
participant Template as TemplateModel
participant DeviceCert as DeviceCertificate
participant X509 as django-x509.Cert
Client->>Config: assign cert Template (m2m add)
Config->>DeviceCert: manage_device_certs (post_add) -> get_or_create DeviceCertificate
DeviceCert->>Template: read ca, blueprint_cert, auto_cert
DeviceCert->>X509: _auto_create_cert (clone blueprint or CA defaults)
X509-->>DeviceCert: saved Cert instance assigned
Client->>Config: unassign cert Template (m2m remove)
Config->>DeviceCert: delete DeviceCertificate
DeviceCert->>X509: post_delete -> revoke() if auto_cert
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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)
Fix these issues in Kilo Cloud Reviewed by gpt-5.4-20260305 · 41,749 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.404% — 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
Aryamanz29
left a comment
Aryamanz29
left a comment
There was a problem hiding this comment.
Good progress! @stktyagi - requested changes below:
Tip: also, don't hesitate to add one-liner comments to complex if-else ladders, or abstract them out into small functions/methods for reusability. It helps us follow the code more easily and keeps things maintainable!
| Automatically creates and assigns a client x509 certificate | ||
| using Blueprint cloning and custom hardware OID injection. | ||
| """ | ||
| import copy |
There was a problem hiding this comment.
could you please move inline imports to top of the file 🙏
| def save(self, *args, **kwargs): | ||
| """Performs automatic provisioning if ``auto_cert`` is True.""" | ||
| if self.auto_cert and not self.cert: | ||
| self._auto_x509() | ||
| super().save(*args, **kwargs) |
There was a problem hiding this comment.
_auto_create_cert is not transactional ^
cert.save() runs, then super().save() (the DeviceCertificate). If the second save raises (constraint, signal handler error), you've leaked a Cert row with no DeviceCertificate pointing at it. Should be wrapped in transaction.atomic() eg:
from django.db import transaction
def save(self, *args, **kwargs):
with transaction.atomic():
if self.auto_cert and not self.cert:
self._auto_x509()
super().save(*args, **kwargs)| # 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" | ||
| # extensions.extend([ | ||
| # { | ||
| # "oid": mac_oid, | ||
| # "value": f"ASN1:UTF8:string:{device.mac_address}", | ||
| # "critical": False | ||
| # }, | ||
| # { | ||
| # "oid": uuid_oid, | ||
| # "value": f"ASN1:UTF8:string:{device.id}", | ||
| # "critical": False | ||
| # } | ||
| # ]) |
There was a problem hiding this comment.
^ Dead commented-out code committed
There was a problem hiding this comment.
this is blocked by openwisp/django-x509#228 so i commented it out to make it easier to implement later and just uncomment. If that seems better i can remove it for now.
| # uncomment after #228 in django-x509 | ||
| # with self.subTest("Injects custom MAC and UUID OIDs"): | ||
| # extensions = generated_cert.extensions | ||
| # mac_oid = "1.3.6.1.4.1.65901.1" | ||
| # uuid_oid = "1.3.6.1.4.1.65901.2" | ||
| # mac_ext = next( | ||
| # ( | ||
| # ext | ||
| # for ext in extensions | ||
| # if ext.get("oid") == mac_oid | ||
| # or ext.get("name") == mac_oid | ||
| # ), | ||
| # None, | ||
| # ) | ||
| # uuid_ext = next( | ||
| # ( | ||
| # ext | ||
| # for ext in extensions | ||
| # if ext.get("oid") == uuid_oid | ||
| # or ext.get("name") == uuid_oid | ||
| # ), | ||
| # None, | ||
| # ) | ||
| # self.assertIsNotNone(mac_ext, "MAC OID extension missing") | ||
| # self.assertIn(device.mac_address, mac_ext["value"]) | ||
| # self.assertIsNotNone(uuid_ext, "UUID OID extension missing") | ||
| # self.assertIn(str(device.id), uuid_ext["value"]) |
There was a problem hiding this comment.
Dead commented-out code committed
There was a problem hiding this comment.
same here, this test is to verify custom oid implementation currently blocked by openwisp/django-x509#228
| def manage_device_certs(cls, sender, instance, action, pk_set, **kwargs): | ||
| """ | ||
| Syncs DeviceCertificate objects when templates are added/removed | ||
| """ | ||
| if instance._state.adding or action not in [ | ||
| "post_add", | ||
| "post_remove", | ||
| "post_clear", | ||
| ]: | ||
| return | ||
| if action == "post_clear": | ||
| if instance.is_deactivating_or_deactivated(): | ||
| instance.devicecertificate_set.all().delete() | ||
| return | ||
| if isinstance(pk_set, set): | ||
| template_model = cls.get_template_model() | ||
| templates = template_model.objects.filter(pk__in=list(pk_set)).order_by( | ||
| "created" | ||
| ) | ||
| else: | ||
| templates = pk_set | ||
| if len(pk_set) != templates.filter(required=True).count(): | ||
| instance.devicecertificate_set.exclude( | ||
| template_id__in=instance.templates.values_list("id", flat=True) | ||
| ).delete() | ||
| if action == "post_add": | ||
| for template in templates.filter(type="cert"): | ||
| instance.devicecertificate_set.get_or_create( | ||
| template=template, defaults={"auto_cert": template.auto_cert} | ||
| ) |
There was a problem hiding this comment.
can you please add a code comment to explain this if-else ladder in manage_device_certs? 🙏
| if ( | ||
| current.ca_id != self.ca_id | ||
| or current.blueprint_cert_id != self.blueprint_cert_id | ||
| ): |
There was a problem hiding this comment.
I think we only blocks ca_id / blueprint_cert_id mutation here. If a user keeps ca the same but changes type from cert -> generic, _clean_cert_template then sets ca = None and blueprint_cert = None on the active template - bypassing the protection and leaving DeviceCertificate rows that won't get cleaned up by the template save (only m2m signals clean them). Either block type-change-off-cert on active templates or trigger the cert cleanup explicitly.
| extra_kwargs = { | ||
| "blueprint_cert": { | ||
| "error_messages": { | ||
| "does_not_exist": _( | ||
| "This certificate does not exist or is already " | ||
| "assigned to an active device." | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Misleading error message^
does_not_exist message says "already assigned to an active device" but the queryset filter (get_unassigned_certs()) excludes any cert with a DeviceCertificate, regardless of whether the device is active/deactivated. Either tighten the filter or relax the message.
There was a problem hiding this comment.
Thank you for this one, let me share a bit of context on why I did that in the first place:
If a user tries to assign a certificate that is already in use, DRF falls back to its native, generic relational error:
{'blueprint_cert': [ErrorDetail(string='Invalid pk "37" - object does not exist.', code='does_not_exist')]} so to avoid unhelpful error message I added that but you are right and we can improve the wording.
I will update the string to:
"This certificate does not exist or is already assigned to a device configuration profile."
This keeps our helpful error override intact while making the text technically accurate. If you have any other suggestion to do this better I'll appreciate that 🙏
| def validate(self, data): | ||
| """ | ||
| Explicitly validate certificate template fields and locks for the API. | ||
| """ |
There was a problem hiding this comment.
can you please add a code comment below for each if-else ladder in validate()? 🙏
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
- Wrap DeviceCertificate.save() in transaction.atomic() to ensure automatic X.509 provisioning does not leak orphaned rows on failure. - Add validation to block changing the template type away from 'cert' on templates currently assigned to active devices.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/config/base/config.py (1)
518-522:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
post_clearstill leaks staleDeviceCertificaterows on real clears.For active configs this branch returns without any reconciliation, so any authoritative
templates.clear()path leaves olddevicecertificate_setrows behind.DeviceDetailSerializer.update()still does exactly that during organization changes, and directconfig.templates.clear()has the same effect, so certificates can remain linked and unreconciled even though their templates are no longer assigned. Please add an explicit cleanup path for non-reorder clears, or pass enough context to distinguish sortedm2m reordering from a real clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/config/base/config.py` around lines 518 - 522, The post_clear branch currently only deletes DeviceCertificate rows when instance.is_deactivating_or_deactivated() is true, which allows real clears (e.g., Config.templates.clear() or DeviceDetailSerializer.update() during org changes) to leak stale devicecertificate_set entries; modify the handler for action == "post_clear" in the config cleanup logic so that it also performs an explicit reconciliation/cleanup of instance.devicecertificate_set for non-deactivate clears (or use passed context to detect a sortedm2m reorder vs a true clear) — specifically, update the post_clear logic that checks instance.is_deactivating_or_deactivated() to also remove or reconcile certificates when templates are cleared (and/or accept a flag from DeviceDetailSerializer.update()/templates.clear() to indicate a real clear) so stale DeviceCertificate rows are deleted.
🤖 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 40-44: get_unassigned_certs() currently builds assigned_cert_ids
from DeviceCertificate.objects.values_list("cert_id", flat=True) which can
include NULL and break the exclude(id__in=...) query; update the query to omit
NULL cert IDs (e.g., use
DeviceCertificate.objects.exclude(cert_id__isnull=True).values_list("cert_id",
flat=True) or .filter(cert_id__isnull=False).values_list(...)) so
assigned_cert_ids contains only real IDs before using it in
Cert.objects.exclude(id__in=assigned_cert_ids) in get_unassigned_certs().
---
Duplicate comments:
In `@openwisp_controller/config/base/config.py`:
- Around line 518-522: The post_clear branch currently only deletes
DeviceCertificate rows when instance.is_deactivating_or_deactivated() is true,
which allows real clears (e.g., Config.templates.clear() or
DeviceDetailSerializer.update() during org changes) to leak stale
devicecertificate_set entries; modify the handler for action == "post_clear" in
the config cleanup logic so that it also performs an explicit
reconciliation/cleanup of instance.devicecertificate_set for non-deactivate
clears (or use passed context to detect a sortedm2m reorder vs a true clear) —
specifically, update the post_clear logic that checks
instance.is_deactivating_or_deactivated() to also remove or reconcile
certificates when templates are cleared (and/or accept a flag from
DeviceDetailSerializer.update()/templates.clear() to indicate a real clear) so
stale DeviceCertificate rows are deleted.
🪄 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: 4b38b811-4f6b-4a60-91c5-a30808887443
📒 Files selected for processing (16)
openwisp_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/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_api.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
⏰ 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~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.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:
tests/openwisp2/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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
**/*.{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/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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
**/*.{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/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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
**/*.{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/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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
🧠 Learnings (4)
📚 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/settings.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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
📚 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/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.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/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/apps.pyopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/template.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.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 (16)
tests/openwisp2/settings.py (1)
275-275: LGTM!openwisp_controller/config/static/config/js/template_ui.js (1)
1-25: LGTM!openwisp_controller/config/admin.py (2)
118-126: LGTM!
1067-1068: LGTM!Also applies to: 1083-1083, 1090-1091, 1103-1103
openwisp_controller/config/apps.py (2)
72-72: LGTM!
98-102: LGTM!Also applies to: 124-128
openwisp_controller/config/tests/test_selenium.py (1)
772-805: LGTM!openwisp_controller/config/base/device_certificate.py (4)
13-52: LGTM!
54-84: LGTM!
86-151: LGTM!
153-163: LGTM!tests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.py (1)
13-150: LGTM!openwisp_controller/pki/tests/test_api.py (1)
155-155: ⚡ Quick winConfirm the delete query bump matches Django’s related-object handling
openwisp_controller/pki/tests/test_api.pynow expects more queries for deletes (test_ca_delete_api: 6,test_cert_delete_api: 8). With the new relations:
DeviceCertificate.cert→django_x509.Certuseson_delete=CASCADETemplate.blueprint_cert→django_x509.Certuseson_delete=SET_NULLDeviceCertificatehas apost_deletehook that can callinstance.cert.revoke()whenauto_cert=TrueThat explains why cert deletion can add more work than CA deletion (CASCADE on
DeviceCertificate.certplus SET_NULL updates forTemplate.blueprint_cert). Sincepki/tests/test_api.pydoesn’t explicitly createTemplate/DeviceCertificateobjects, the increase should be coming from Django’s delete collector existence checks/SET_NULL handling—not per-row churn.Check the actual SQL for these two DELETE endpoints and confirm the extra queries are constant-time (no N+1) even when multiple related
DeviceCertificate/Templaterows exist.openwisp_controller/config/base/template.py (1)
25-29: LGTM!Also applies to: 69-92, 128-129, 251-341, 351-355, 369-370, 378-378
openwisp_controller/config/tests/test_api.py (1)
33-33: LGTM!Also applies to: 742-742, 1333-1540
openwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.py (1)
1-152: LGTM!
Fixed NULL handling in get_unassigned_certs() to avoid empty blueprint_cert choices
|
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). |
Updated get_unassigned_certs to include revoked certificates
Dependency Conflict in
|
…s/1356-extend-abstract-template
|
@coderabbitai any outstanding issue? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get all review comments on PR `#1378`
gh pr view 1378 --repo openwisp/openwisp-controller --json reviews,comments | jq '.'Length of output: 229590 🏁 Script executed: #!/bin/bash
# Get review comments (inline) on PR `#1378`
gh api repos/openwisp/openwisp-controller/pulls/1378/comments \
--paginate \
| jq '[.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, path: .path, line: .line, body: .body[0:300]}]'Length of output: 21777 🏁 Script executed: #!/bin/bash
# Get all issue comments on PR `#1378` to find CodeRabbit summary/walkthrough
gh api repos/openwisp/openwisp-controller/issues/1378/comments \
--paginate \
| jq '[.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, created_at: .created_at, body: .body[0:500]}]'Length of output: 3351
❌ Pre-merge failure (blocker)
In Fix: # change:
assigned_cert_ids = DeviceCertificate.objects.values_list("cert_id", flat=True)
# to:
assigned_cert_ids = DeviceCertificate.objects.filter(
cert_id__isnull=False
).values_list("cert_id", flat=True)A regression test should create a
|
…s/1356-extend-abstract-template
Updated post_clear to prevent stale DeviceCertificate rows
|
@coderabbitai full review or resolve |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of get_unassigned_certs in template.py
rg -n "get_unassigned_certs|cert_id__isnull|assigned_cert_ids" openwisp_controller/config/base/template.py
echo "---"
# Check post_clear branch in config.py
rg -n "post_clear|post_add|post_remove|manage_device_certs|is_deactivating" openwisp_controller/config/base/config.py -A 5
echo "---"
# List open CodeRabbit inline review comments
gh api repos/openwisp/openwisp-controller/pulls/1378/comments \
--paginate \
| jq '[.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, path: .path, line: .line, body: .body[0:200]}]'Length of output: 23572
There are still several open inline comments from the most recent review rounds (ranging from nitpicks to majors). Triggering a fresh full review now to get an up-to-date assessment of everything remaining. ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_controller/config/base/template.py (1)
359-375:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
vpnbefore validating its organization scope.
self._validate_org_relation("vpn")still runs before Lines 372-375 null outself.vpn. A save that changes a template from"vpn"to"cert"/"generic"and moves it to another organization can still fail on the stale VPN relation even though this code would clear it later in the sameclean()call. Move the VPN org check inside theself.type == "vpn"branch, or run it after the non-VPN cleanup.🐛 Minimal fix
- self._validate_org_relation("vpn") if not self.default_values: self.default_values = {} @@ - if self.type == "vpn" and not self.vpn: - raise ValidationError( - {"vpn": _('A VPN must be selected when template type is "VPN"')} - ) - elif self.type != "vpn": + if self.type == "vpn": + self._validate_org_relation("vpn") + if not self.vpn: + raise ValidationError( + {"vpn": _('A VPN must be selected when template type is "VPN"')} + ) + else: self.vpn = None if self.type != "cert": self.auto_cert = False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/config/base/template.py` around lines 359 - 375, The VPN organization relation check is performed too early (self._validate_org_relation("vpn")) before the code clears self.vpn for non-VPN templates, causing stale VPN relations to block saves; move the VPN-specific validation so it only runs when the template type is "vpn" (i.e., call self._validate_org_relation("vpn") inside the self.type == "vpn" branch) or alternatively run it after the block that sets self.vpn = None/auto_cert = False; update the logic around the type checks in the clean()/validation path (references: _validate_cert_template_changes, _clean_cert_template, _validate_org_relation("vpn"), and the conditional branches using self.type) so non-VPN templates have their vpn cleared before any organization-relation validation runs.openwisp_controller/config/api/serializers.py (1)
74-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the instance type in
validate_config()for partial updates.On PATCH requests that only send
config,self.initial_data.get("type")isNone, so existing"generic"templates miss this field-level check and fall back to the model-level non-field error instead. Please fall back toself.instance.typehere as well.💡 Minimal fix
- template_type = self.initial_data.get("type") + template_type = self.initial_data.get( + "type", getattr(self.instance, "type", None) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/config/api/serializers.py` around lines 74 - 83, validate_config currently only reads template_type from self.initial_data, which is None on PATCH requests that only include config; update validate_config to fall back to the serializer instance's type when initial_data lacks it (e.g., template_type = self.initial_data.get("type") or (getattr(self.instance, "type", None))) and ensure you handle self.instance being None before accessing its type so the empty-{} check for "generic" templates still runs on partial updates.
♻️ Duplicate comments (1)
openwisp_controller/config/api/serializers.py (1)
104-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the active-template lock keyed to the original request fields.
Lines 105-107 always inject
ca=Noneandblueprint_cert=Nonefor non-cert updates, so the guard at Lines 122-126 becomes true even for unrelated PATCHes. That reintroduces the extraConfig.objects.filter(...).exists()query on every update of an assigned non-cert template. Capture whethertype/ca/blueprint_certwere actually submitted before normalizingdata, and use that flag for the lock check.♻️ Minimal fix
template_type = data.get("type", getattr(self.instance, "type", "generic")) + protected_fields_supplied = any( + field in data for field in ["type", "ca", "blueprint_cert"] + ) ca = data.get("ca", getattr(self.instance, "ca", None)) blueprint_cert = data.get( "blueprint_cert", getattr(self.instance, "blueprint_cert", None) ) @@ if ( self.instance and self.instance.pk - and ("ca" in data or "blueprint_cert" in data or "type" in data) + and protected_fields_supplied ):Also applies to: 121-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/config/api/serializers.py` around lines 104 - 107, The serializer currently unconditionally sets data["ca"]=None and data["blueprint_cert"]=None when template_type != "cert", which causes the active-template lock check (the Config.objects.filter(...).exists() branch) to trigger for unrelated PATCHes; instead, detect whether the incoming request actually included any of the template key fields before normalization (e.g., check for presence of "type", "ca", or "blueprint_cert" in the input payload), store a boolean flag like submitted_template_fields, then perform the normalization (setting to None) but only run the lock existence check when submitted_template_fields is True; apply the same presence-flag logic for the other similar block around the 121-133 region so the extra DB query runs only when template-related fields were actually submitted.
🤖 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/tests/test_template.py`:
- Around line 1335-1375: Add a new test that covers the partial-replacement
path: when config.templates.set([...], clear=True) removes a cert template but
leaves other templates selected, ensure the existing DeviceCertificate is not
deleted/revoked; implement it alongside
test_cert_template_reorder_does_not_revoke as e.g.
test_cert_template_partial_replace_does_not_revoke, create
org/ca/templates/device/config, call config.templates.set to establish a cert
then call config.templates.set with a new list that drops the cert-template
while keeping other templates, and after the operation assert the
DeviceCertificate (config.devicecertificate_set.first()) still has the same pk,
the underlying cert pk is unchanged, and cert.revoked is False to exercise the
post_clear / transaction.on_commit branch in
openwisp_controller/config/base/config.py.
---
Outside diff comments:
In `@openwisp_controller/config/api/serializers.py`:
- Around line 74-83: validate_config currently only reads template_type from
self.initial_data, which is None on PATCH requests that only include config;
update validate_config to fall back to the serializer instance's type when
initial_data lacks it (e.g., template_type = self.initial_data.get("type") or
(getattr(self.instance, "type", None))) and ensure you handle self.instance
being None before accessing its type so the empty-{} check for "generic"
templates still runs on partial updates.
In `@openwisp_controller/config/base/template.py`:
- Around line 359-375: The VPN organization relation check is performed too
early (self._validate_org_relation("vpn")) before the code clears self.vpn for
non-VPN templates, causing stale VPN relations to block saves; move the
VPN-specific validation so it only runs when the template type is "vpn" (i.e.,
call self._validate_org_relation("vpn") inside the self.type == "vpn" branch) or
alternatively run it after the block that sets self.vpn = None/auto_cert =
False; update the logic around the type checks in the clean()/validation path
(references: _validate_cert_template_changes, _clean_cert_template,
_validate_org_relation("vpn"), and the conditional branches using self.type) so
non-VPN templates have their vpn cleared before any organization-relation
validation runs.
---
Duplicate comments:
In `@openwisp_controller/config/api/serializers.py`:
- Around line 104-107: The serializer currently unconditionally sets
data["ca"]=None and data["blueprint_cert"]=None when template_type != "cert",
which causes the active-template lock check (the
Config.objects.filter(...).exists() branch) to trigger for unrelated PATCHes;
instead, detect whether the incoming request actually included any of the
template key fields before normalization (e.g., check for presence of "type",
"ca", or "blueprint_cert" in the input payload), store a boolean flag like
submitted_template_fields, then perform the normalization (setting to None) but
only run the lock existence check when submitted_template_fields is True; apply
the same presence-flag logic for the other similar block around the 121-133
region so the extra DB query runs only when template-related fields were
actually submitted.
🪄 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: cdcf57db-c535-46f4-aaf1-4830eecceb77
📒 Files selected for processing (16)
openwisp_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/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/models.pyopenwisp_controller/config/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_api.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
⏰ 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.11 | django~=5.2.0
- GitHub Check: Python==3.12 | 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.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
🧰 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/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/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/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/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/static/config/js/template_ui.jsopenwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/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/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.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/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/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/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/models.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/template.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_selenium.pyopenwisp_controller/config/apps.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/models.pytests/openwisp2/settings.pyopenwisp_controller/config/admin.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.pytests/openwisp2/sample_config/migrations/0010_template_blueprint_cert_template_ca_and_more.pytests/openwisp2/sample_config/models.pyopenwisp_controller/config/migrations/0064_template_blueprint_cert_template_ca_and_more.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/tests/test_template.pyopenwisp_controller/config/base/device_certificate.pyopenwisp_controller/config/base/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 (17)
openwisp_controller/config/admin.py (1)
120-125: LGTM!Also applies to: 1067-1068, 1083-1083, 1090-1091, 1103-1103
openwisp_controller/config/static/config/js/template_ui.js (1)
1-24: LGTM!openwisp_controller/config/base/config.py (1)
66-72: LGTM!Also applies to: 505-554
openwisp_controller/config/apps.py (1)
72-72: LGTM!Also applies to: 98-128
openwisp_controller/config/tests/test_api.py (10)
33-33: LGTM!
742-742: LGTM!
1333-1351: LGTM!
1352-1368: LGTM!
1369-1389: LGTM!
1390-1414: LGTM!
1415-1447: LGTM!
1448-1482: LGTM!
1483-1503: LGTM!
1504-1540: LGTM!openwisp_controller/config/tests/test_selenium.py (1)
772-804: LGTM!openwisp_controller/pki/tests/test_api.py (2)
155-155: LGTM!
275-275: LGTM!
A companion regression for “old cert removed, other templates kept”
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@stktyagi start drafting a bit of docs, nothing major but start drafting so we keep on iterating on it in the next PRs.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
GPT 5.5 assisted review.
Findings
Medium
docs/user/settings.rst:310 and docs/user/rest-api.rst:1114
The code adds the new cert template type and allows auto_cert for certificate templates, but the user docs still say auto_cert is valid only for VPN templates and the template API docs still list only vpn or generic as type examples. This will confuse users trying to use the new feature from the admin or API, and it also leaves the current maintainer request for minimal docs unresolved.
I would add a small section to docs/user/templates.rst explaining certificate templates, the required CA, optional blueprint certificate, and what happens when the template is assigned or removed. I would also update docs/user/settings.rst and docs/user/rest-api.rst so cert is documented as a valid template type.
Notes
I do not see a remaining blocking code issue in the current branch. The two CodeRabbit blockers about get_unassigned_certs() NULL handling and stale DeviceCertificate rows after post_clear are addressed in the latest code and covered by regression tests.
…s/1356-extend-abstract-template
Uncommented code for custom oid extensions used for mac address and uuid support in certificate with relevant test Fixes #1377
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
4 participants
Checklist
Reference to Existing Issue
Closes #1356
Description of Changes
This PR establishes the database architecture, UI, API and lifecycle for standalone X.509 certificate templates.
Closes #1356
Closes #1377
Closes #1357
Closes #1361
Closes #1358
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.output.mp4