[feature] Added "x509_renew" signal for Ca and Cert model #236#237
Conversation
pandafy
added this to
OpenWISP Priorities for next releases and OpenWISP Contributor's Board
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR adds the Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant BaseX509
participant transaction
participant x509_renewed as x509_renewed Signal
BaseX509->>BaseX509: renew()
BaseX509->>transaction: on_commit(send_robust)
transaction->>x509_renewed: send_robust(sender, instance)
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
|
coverage: 95.789% (+0.03%) from 95.758% — issues/236-renew-signals into master |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
django_x509/base/models.py (1)
879-887: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
send_robust()for the renewal signal.
A receiver exception fromx509_renewed.send()will abortAbstractCa.renew()before the remaining certs are renewed.Proposed fix
- x509_renewed.send(sender=self.__class__, instance=self) + x509_renewed.send_robust(sender=self.__class__, instance=self)🤖 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 `@django_x509/base/models.py` around lines 879 - 887, In AbstractCa.renew, the x509_renewed signal is sent with send(), which lets a receiver exception abort the renewal flow; switch this to send_robust() so one failing receiver does not stop remaining certificates from being renewed. Update the signal dispatch in renew() after self.save() and keep the rest of the renewal logic unchanged.
🤖 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 `@README.rst`:
- Around line 417-435: The x509_renewed signal documentation should warn
consumers not to log or serialize the full instance, since the Ca/Cert object
can include private_key and passphrase data. Update the Signals section for
django_x509.signals.x509_renewed to add a brief safety note near the instance
argument description, explicitly referencing Ca and Cert so downstream handlers
avoid accidental secret exposure.
---
Outside diff comments:
In `@django_x509/base/models.py`:
- Around line 879-887: In AbstractCa.renew, the x509_renewed signal is sent with
send(), which lets a receiver exception abort the renewal flow; switch this to
send_robust() so one failing receiver does not stop remaining certificates from
being renewed. Update the signal dispatch in renew() after self.save() and keep
the rest of the renewal logic unchanged.
🪄 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: 10553e06-68ab-4c72-ab53-6ebc0433655d
📒 Files selected for processing (5)
README.rstdjango_x509/base/models.pydjango_x509/signals.pydjango_x509/tests/test_ca.pydjango_x509/tests/test_cert.py
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: Python==3.13 | 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~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.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.12 | django~=5.2.0
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
{README.rst,docs/**}
📄 CodeRabbit inference engine (AGENTS.md)
Update documentation when behavior, settings, public APIs, setup steps, or supported versions change
Files:
README.rst
**/*.py
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Preserve object-level permissions and swappable model support when present
Be careful with certificate authority state, certificate revocation, serial numbers, extensions, private key handling, admin actions, and migrations
Watch for private key exposure, unsafe file paths, weak certificate options, invalid extensions, and secrets
Preserve validation around CA material, certificate material, revocation, downloads, and uploaded/generated files
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
django_x509/signals.pydjango_x509/base/models.pydjango_x509/tests/test_cert.pydjango_x509/tests/test_ca.py
{django_x509/tests/**,tests/**}
📄 CodeRabbit inference engine (AGENTS.md)
{django_x509/tests/**,tests/**}: Add or update tests for every behavior change
For bug fixes, write the regression test first, run it against the unfixed code, confirm it fails for the expected reason, then implement the fix
Prefer in-process tests so coverage tools can measure changed code
Files:
django_x509/tests/test_cert.pydjango_x509/tests/test_ca.py
🧠 Learnings (1)
📚 Learning: 2026-01-16T21:54:25.644Z
Learnt from: stktyagi
Repo: openwisp/django-x509 PR: 201
File: tests/openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py:18-26
Timestamp: 2026-01-16T21:54:25.644Z
Learning: Do not review or suggest adding 224-bit ECDSA support in django-x509 code. SECP224R1 provides only 112 bits of cryptographic strength and is deprecated for asymmetric use by NIST SP 800-131A Rev.3 after 2030. Prefer and enforce the use of modern curves (e.g., SECP256R1/P-256) as the minimum. This guideline applies broadly to Python files in this repository; ensure reviews advocate using at least 256-bit curves and document any cryptographic choices accordingly.
Applied to files:
django_x509/signals.pydjango_x509/base/models.pydjango_x509/tests/test_cert.pydjango_x509/tests/test_ca.py
🔇 Additional comments (4)
django_x509/signals.py (1)
1-6: LGTM!django_x509/base/models.py (1)
20-20: LGTM!django_x509/tests/test_ca.py (1)
13-16: LGTM!Also applies to: 803-819
django_x509/tests/test_cert.py (1)
11-16: LGTM!Also applies to: 882-890
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
|
@coderabbitai resolve |
github-project-automation
Bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
✅ Action performedComments resolved and changes approved. |
|
Proposed change log entry: |
| self.validity_end = default_ca_validity_end() | ||
| self._generate() | ||
| self.save() | ||
| x509_renewed.send_robust(sender=self.__class__, instance=self) |
There was a problem hiding this comment.
@pandafy this sends the signal while the DB transaction may be active, are we aware of this and is it on purpose right? This means the receivers have to execute when the transaction is committed. I'd leave a notes about this in the docs.
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)
django_x509/base/models.py (1)
879-889: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a brief "why" comment for the deferred signal emission.
The deferred, commit-gated emission is non-obvious (it already prompted a reviewer question about whether this was intentional). A short comment explaining that the signal is fired after commit so receivers never see a state that could be rolled back would help future readers avoid re-litigating this.
As per coding guidelines, "Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it."📝 Suggested comment
self._generate() self.save() + # Deferred until commit so receivers never observe a renewal that is + # later rolled back (see README "Signals" section for full contract). transaction.on_commit( lambda: x509_renewed.send_robust(sender=self.__class__, instance=self) )🤖 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 `@django_x509/base/models.py` around lines 879 - 889, Add a brief why-comment in renew() right before the transaction.on_commit block, explaining that x509_renewed is emitted only after the transaction commits so receivers never observe a state that could still be rolled back. Keep the comment adjacent to the deferred signal emission and reference the existing renew() flow and x509_renewed.send_robust call so future readers understand the commit-gated behavior.Source: Coding guidelines
django_x509/tests/test_ca.py (1)
803-820: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing regression test for the rollback-discards-signal claim.
README now documents: "If the transaction is rolled back... the signal will not be sent at all," but neither this file nor
test_cert.pyhas a test assertingx509_renewedis not sent when the surrounding transaction rolls back. Since this is a new, explicitly documented behavior, it's worth a dedicated regression test (e.g., wrapca.renew()intransaction.atomic()and force a rollback via an exception, then assert the captured on-commit callback list is empty or the signal handler was never invoked).As per path instructions, "Add or update tests for every behavior change."🧪 Suggested test sketch
+ def test_x509_renewed_signal_not_sent_on_rollback(self): + ca = self._create_ca() + with catch_signal(x509_renewed) as handler: + with self.assertRaises(RuntimeError): + with transaction.atomic(): + ca.renew() + raise RuntimeError("force rollback") + handler.assert_not_called()🤖 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 `@django_x509/tests/test_ca.py` around lines 803 - 820, Add a regression test for the rollback behavior of x509_renewed around Ca.renew(): the current tests only cover the committed path in test_x509_renewed_signal_sent, but not the documented case where a transaction rollback prevents the signal from being emitted. In django_x509/tests/test_ca.py (or the related cert tests if that is where the transaction boundary is clearer), wrap ca.renew() in transaction.atomic(), force a rollback with an exception, and assert that the catch_signal(x509_renewed) handler was never called or that captureOnCommitCallbacks(execute=False) stays empty.Source: Path instructions
🤖 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 `@django_x509/base/models.py`:
- Around line 887-889: Capture and handle the return value from
x509_renewed.send_robust inside the transaction.on_commit callback instead of
discarding it. The issue is that receiver exceptions are being swallowed
silently, so update the lambda around x509_renewed.send_robust to inspect the
`(receiver, response)` results and log any exception responses with enough
context to identify the failing receiver and instance. Use the existing
x509_renewed signal and the on_commit callback in django_x509/base/models.py as
the place to add the logging.
---
Outside diff comments:
In `@django_x509/base/models.py`:
- Around line 879-889: Add a brief why-comment in renew() right before the
transaction.on_commit block, explaining that x509_renewed is emitted only after
the transaction commits so receivers never observe a state that could still be
rolled back. Keep the comment adjacent to the deferred signal emission and
reference the existing renew() flow and x509_renewed.send_robust call so future
readers understand the commit-gated behavior.
In `@django_x509/tests/test_ca.py`:
- Around line 803-820: Add a regression test for the rollback behavior of
x509_renewed around Ca.renew(): the current tests only cover the committed path
in test_x509_renewed_signal_sent, but not the documented case where a
transaction rollback prevents the signal from being emitted. In
django_x509/tests/test_ca.py (or the related cert tests if that is where the
transaction boundary is clearer), wrap ca.renew() in transaction.atomic(), force
a rollback with an exception, and assert that the catch_signal(x509_renewed)
handler was never called or that captureOnCommitCallbacks(execute=False) stays
empty.
🪄 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: 82eb461f-0c40-485a-a916-84ea925730a8
📒 Files selected for processing (4)
README.rstdjango_x509/base/models.pydjango_x509/tests/test_ca.pydjango_x509/tests/test_cert.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Preserve object-level permissions and swappable model support when present
Be careful with certificate authority state, certificate revocation, serial numbers, extensions, private key handling, admin actions, and migrations
Watch for private key exposure, unsafe file paths, weak certificate options, invalid extensions, and secrets
Preserve validation around CA material, certificate material, revocation, downloads, and uploaded/generated files
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
django_x509/tests/test_cert.pydjango_x509/tests/test_ca.pydjango_x509/base/models.py
{django_x509/tests/**,tests/**}
📄 CodeRabbit inference engine (AGENTS.md)
{django_x509/tests/**,tests/**}: Add or update tests for every behavior change
For bug fixes, write the regression test first, run it against the unfixed code, confirm it fails for the expected reason, then implement the fix
Prefer in-process tests so coverage tools can measure changed code
Files:
django_x509/tests/test_cert.pydjango_x509/tests/test_ca.py
{README.rst,docs/**}
📄 CodeRabbit inference engine (AGENTS.md)
Update documentation when behavior, settings, public APIs, setup steps, or supported versions change
Files:
README.rst
🧠 Learnings (1)
📚 Learning: 2026-01-16T21:54:25.644Z
Learnt from: stktyagi
Repo: openwisp/django-x509 PR: 201
File: tests/openwisp2/sample_x509/migrations/0003_alter_ca_key_length_alter_cert_key_length_and_more.py:18-26
Timestamp: 2026-01-16T21:54:25.644Z
Learning: Do not review or suggest adding 224-bit ECDSA support in django-x509 code. SECP224R1 provides only 112 bits of cryptographic strength and is deprecated for asymmetric use by NIST SP 800-131A Rev.3 after 2030. Prefer and enforce the use of modern curves (e.g., SECP256R1/P-256) as the minimum. This guideline applies broadly to Python files in this repository; ensure reviews advocate using at least 256-bit curves and document any cryptographic choices accordingly.
Applied to files:
django_x509/tests/test_cert.pydjango_x509/tests/test_ca.pydjango_x509/base/models.py
🔇 Additional comments (4)
django_x509/base/models.py (1)
12-12: LGTM!README.rst (1)
417-449: LGTM!django_x509/tests/test_ca.py (1)
13-16: LGTM!Also applies to: 803-820
django_x509/tests/test_cert.py (1)
11-16: LGTM!Also applies to: 882-891
| transaction.on_commit( | ||
| lambda: x509_renewed.send_robust(sender=self.__class__, instance=self) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Receiver failures from send_robust are silently discarded.
send_robust catches exceptions per-receiver and returns them as (receiver, response) tuples instead of raising, but the return value here isn't captured, so a failing receiver (e.g. a broken logging/workflow hook — exactly the use cases this signal targets) fails silently with no trace anywhere. Consider capturing the result and logging any exceptions.
🔍 Suggested fix
+ def _emit_renewed():
+ for receiver, response in x509_renewed.send_robust(
+ sender=self.__class__, instance=self
+ ):
+ if isinstance(response, Exception):
+ logger.exception(
+ "Error in x509_renewed receiver %r", receiver, exc_info=response
+ )
+
- transaction.on_commit(
- lambda: x509_renewed.send_robust(sender=self.__class__, instance=self)
- )
+ transaction.on_commit(_emit_renewed)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transaction.on_commit( | |
| lambda: x509_renewed.send_robust(sender=self.__class__, instance=self) | |
| ) | |
| def _emit_renewed(): | |
| for receiver, response in x509_renewed.send_robust( | |
| sender=self.__class__, instance=self | |
| ): | |
| if isinstance(response, Exception): | |
| logger.exception( | |
| "Error in x509_renewed receiver %r", receiver, exc_info=response | |
| ) | |
| transaction.on_commit(_emit_renewed) |
🤖 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 `@django_x509/base/models.py` around lines 887 - 889, Capture and handle the
return value from x509_renewed.send_robust inside the transaction.on_commit
callback instead of discarding it. The issue is that receiver exceptions are
being swallowed silently, so update the lambda around x509_renewed.send_robust
to inspect the `(receiver, response)` results and log any exception responses
with enough context to identify the failing receiver and instance. Use the
existing x509_renewed signal and the on_commit callback in
django_x509/base/models.py as the place to add the logging.
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Priorities for next releases
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
4 participants
Checklist
Reference to Existing Issue
Closes #236