[feature] Add expiration notices and expired status #202#217
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds expiration monitoring and notification for CAs and certificates: a daily Celery task discovers soon-to-expire and expired objects, emits two Django signals ( Sequence Diagram(s)sequenceDiagram
participant Task as Celery Task (check_x509_expiration)
participant DB as Database
participant Signal as Django Signals
participant Handler as Notification Handler
participant Email as Email Backend
participant Admin as Django Admin
Task->>DB: Query expiring CAs/Certs (notice window)
Task->>Signal: Emit x509_objects_expiring(expiring_cas, expiring_certs, notice_days)
Signal->>Handler: notify_x509_objects_expiring(...)
Handler->>DB: Resolve admin URLs/details
Handler->>Email: send_mass_mail(expiring notice)
Email-->>Handler: delivery result
Task->>DB: Query expired CAs/Certs (transactional, select_for_update)
Task->>Task: Determine auto-renewable vs manual
loop Auto-renew loop
Task->>DB: Renew CA/cert (if allowed)
Task->>DB: Collect renewed / failed lists
end
Task->>Signal: Emit x509_objects_expired(expired_cas, expired_certs, renewed_cas, renewed_certs, failed_cas, failed_certs)
Signal->>Handler: notify_x509_objects_expired(...)
Handler->>Email: send_mass_mail(daily report)
Email-->>Handler: delivery result
Handler->>DB: Mark expire_notified on notified objects
Admin-->>DB: Render cert list (uses is_expired property)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@django_x509/handlers.py`:
- Around line 15-24: _admin_url currently swallows all exceptions and returns an
empty string; update it to catch the exception, log the failure at debug level
(including the exception info and context such as instance._meta.app_label,
instance._meta.model_name and instance.pk) before returning the empty string so
failed URL generation is recorded for diagnosis; keep the reverse call and the
same return behavior but add a debug log entry referencing _admin_url and the
caught exception.
In `@django_x509/tasks.py`:
- Around line 71-109: Wrap the selection and renewal logic in a DB transaction
and claim the rows with SELECT ... FOR UPDATE SKIP LOCKED so concurrent task
runs don't process the same objects: replace the two list(...) queries for
expired_cas and expired_certs with queryset iterations inside a
transaction.atomic() block using .select_for_update(skip_locked=True) (retaining
select_related("ca") for expired_certs), then process each locked Ca and Cert in
that transaction (calling ca.renew() and cert.renew()) and update their
state/expire_notified inside the same transaction; also compute
expired_ca_ids/renewed_ca_ids from the locked rows (use ca.pk and cert.ca_id) so
the cert-renew logic (checks against renewed_ca_ids and expired_ca_ids) sees the
claimed/updated state and avoids double-renewal or races.
- Around line 89-93: The CA auto-renew loop calls ca.renew(), which triggers
AbstractCa.renew() to reissue all related child certificates and thus silently
reissues revoked certs; update the loop to skip revoked child certificates
before renewing by either filtering the CA's child certs and passing only
non-revoked certs into a renew helper or by calling a new/modified renew
signature that accepts exclude_revoked=True, e.g., change the expired_cas loop
to check ca.can_auto_renew_ca then call ca.renew(... ) with an argument or
helper that ensures revoked child certificates (is_revoked / revoked flag on
related cert objects) are excluded from reissuance so their stored cert/serial
history is not overwritten.
- Around line 133-137: The current code marks failed renewals as notified by
calling _mark_expired_objects_notified on entries derived from failed_cas and
failed_certs, which prevents future retry; change the notify/mark logic so that
when notified is true you only call _mark_expired_objects_notified for
manual_expired_cas and manual_expired_certs (i.e., remove the calls that pass
[entry["instance"] for entry in failed_cas] and [entry["instance"] for entry in
failed_certs]), and if you still need failure-email deduplication implement a
separate mechanism (e.g., a distinct flag or table) instead of flipping
expire_notified via _mark_expired_objects_notified for failed renewals.
In `@README.rst`:
- Around line 385-392: Update the README wording to reflect the actual behavior
in handlers.py: state that mail_admins is used when ADMINS is configured and
mail_managers is used when MANAGERS is configured and that both will be used if
both ADMINS and MANAGERS are configured; also clarify that the superuser
fallback (send_mass_mail to superusers with non-empty emails) only runs when
neither ADMINS nor MANAGERS is configured. Mention the exact symbols
mail_admins, mail_managers, ADMINS, MANAGERS, and send_mass_mail so readers can
correlate the docs with handlers.py.
In
`@tests/openwisp2/sample_x509/migrations/0004_ca_auto_renew_ca_expire_notified_and_more.py`:
- Around line 37-41: Remove the redundant migrations.AddField operations that
add the expire_notified BooleanField to CustomCert, Cert and Ca in the migration
file; expire_notified is already defined on BaseX509 (in
django_x509/base/models.py) and inherited via AbstractCa/AbstractCert into
Ca/Cert/CustomCert, so delete the three migrations.AddField entries that
reference name="expire_notified" (the blocks adding expire_notified for ca, cert
and customcert) and leave only the migrations.AddField that adds the auto_renew
field for Ca.
🪄 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: b8e3f6f9-a662-4da6-a972-62446b55c3ff
📒 Files selected for processing (16)
CHANGES.rstREADME.rstdjango_x509/apps.pydjango_x509/base/admin.pydjango_x509/base/models.pydjango_x509/handlers.pydjango_x509/migrations/0011_ca_auto_renew_ca_expire_notified_and_more.pydjango_x509/settings.pydjango_x509/signals.pydjango_x509/tasks.pydjango_x509/tests/test_admin.pydjango_x509/tests/test_ca.pydjango_x509/tests/test_cert.pydjango_x509/tests/test_tasks.pysetup.pytests/openwisp2/sample_x509/migrations/0004_ca_auto_renew_ca_expire_notified_and_more.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 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/settings.pydjango_x509/apps.pydjango_x509/migrations/0011_ca_auto_renew_ca_expire_notified_and_more.pydjango_x509/base/models.pydjango_x509/signals.pydjango_x509/base/admin.pydjango_x509/tests/test_tasks.pydjango_x509/tests/test_ca.pydjango_x509/tests/test_cert.pydjango_x509/tests/test_admin.pydjango_x509/tasks.pysetup.pydjango_x509/handlers.pytests/openwisp2/sample_x509/migrations/0004_ca_auto_renew_ca_expire_notified_and_more.py
📚 Learning: 2026-02-02T19:51:53.987Z
Learnt from: nemesifier
Repo: openwisp/django-x509 PR: 210
File: setup.py:21-24
Timestamp: 2026-02-02T19:51:53.987Z
Learning: In Python projects, it is acceptable during development to reference development branches (e.g., refs/heads/1.3) in install_requires to track compatible development versions. Before a release, switch install_requires to stable, pinned version references to ensure reproducible builds. Document this workflow in contributing guidelines and ensure CI/builds verify that release branches use stable, exact version pins.
Applied to files:
setup.py
🔇 Additional comments (11)
CHANGES.rst (1)
9-26: LGTM!The changelog entries accurately document the new expiration notices feature, the Celery task, Django signals, and the
openwisp-utils[celery]dependency bump. The formatting follows existing conventions.setup.py (1)
21-25: LGTM!Adding the
[celery]extra toopenwisp-utilscorrectly aligns with the new Celery-based expiration task. Based on learnings, the development branch reference is acceptable during development but should be switched to a stable pinned version before release.django_x509/signals.py (1)
1-4: LGTM!Clean signal definitions following Django conventions. The signals are correctly wired in
apps.pyand emitted by the expiration task.README.rst (2)
59-64: LGTM!Feature list updates accurately reflect the new capabilities.
347-384: LGTM!The setting documentation and Celery beat configuration example are clear and follow existing documentation patterns.
tests/openwisp2/sample_x509/migrations/0004_ca_auto_renew_ca_expire_notified_and_more.py (1)
12-36: LGTM!The field definitions for
Ca.auto_renew,Ca.expire_notified, andCert.expire_notifiedcorrectly match the base migration indjango_x509/migrations/0011_ca_auto_renew_ca_expire_notified_and_more.py.django_x509/migrations/0011_ca_auto_renew_ca_expire_notified_and_more.py (1)
1-37: LGTM!The migration correctly adds the
auto_renewandexpire_notifiedfields. The choice values and field parameters match theAutoRenewChoicesenum and model definitions indjango_x509/base/models.py.django_x509/handlers.py (3)
48-81: LGTM with a minor observation.The email notification logic is well-structured with proper fallback from ADMINS → MANAGERS → superusers. The
send_mass_mailusage is correct.One minor note:
getattr(settings, "ADMINS", ())andgetattr(settings, "MANAGERS", ())will return the setting value even if it's an empty tuple, which is truthy in the boolean check context. Since empty tuples are falsy in Python, this works correctly.
84-150: LGTM!The
notify_x509_objects_expiringhandler properly categorizes CAs and certificates into manual vs auto-renewal groups and renders appropriate messages with actionable guidance.
153-237: LGTM!The
notify_x509_objects_expiredhandler comprehensively covers all event categories (renewed, expired, failed) and provides clear status information in the notification email.django_x509/apps.py (1)
9-21: LGTM!The signal wiring follows Django best practices:
- Local imports inside
ready()avoid circular import issuesdispatch_uidprevents duplicate handler registration- Handler-to-signal mapping is correct
| def _admin_url(instance): | ||
| try: | ||
| return reverse( | ||
| "admin:{0}_{1}_change".format( | ||
| instance._meta.app_label, instance._meta.model_name | ||
| ), | ||
| args=[instance.pk], | ||
| ) | ||
| except Exception: | ||
| return "" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging failed URL generation.
The _admin_url function silently returns an empty string on any exception. While this is safe, logging the exception at debug level could help diagnose issues in production.
🔧 Optional: Add debug logging
+import logging
+
+logger = logging.getLogger(__name__)
+
+
def _admin_url(instance):
try:
return reverse(
"admin:{0}_{1}_change".format(
instance._meta.app_label, instance._meta.model_name
),
args=[instance.pk],
)
except Exception:
+ logger.debug(
+ "Could not generate admin URL for %s pk=%s",
+ type(instance).__name__,
+ instance.pk,
+ exc_info=True,
+ )
return ""📝 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.
| def _admin_url(instance): | |
| try: | |
| return reverse( | |
| "admin:{0}_{1}_change".format( | |
| instance._meta.app_label, instance._meta.model_name | |
| ), | |
| args=[instance.pk], | |
| ) | |
| except Exception: | |
| return "" | |
| import logging | |
| logger = logging.getLogger(__name__) | |
| def _admin_url(instance): | |
| try: | |
| return reverse( | |
| "admin:{0}_{1}_change".format( | |
| instance._meta.app_label, instance._meta.model_name | |
| ), | |
| args=[instance.pk], | |
| ) | |
| except Exception: | |
| logger.debug( | |
| "Could not generate admin URL for %s pk=%s", | |
| type(instance).__name__, | |
| instance.pk, | |
| exc_info=True, | |
| ) | |
| return "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@django_x509/handlers.py` around lines 15 - 24, _admin_url currently swallows
all exceptions and returns an empty string; update it to catch the exception,
log the failure at debug level (including the exception info and context such as
instance._meta.app_label, instance._meta.model_name and instance.pk) before
returning the empty string so failed URL generation is recorded for diagnosis;
keep the reverse call and the same return behavior but add a debug log entry
referencing _admin_url and the caught exception.
| expired_cas = list( | ||
| Ca.objects.filter(validity_end__lte=now) | ||
| .filter(Q(expire_notified__isnull=True) | Q(expire_notified=False)) | ||
| .order_by("validity_end", "pk") | ||
| ) | ||
| expired_certs = list( | ||
| Cert.objects.select_related("ca") | ||
| .filter(revoked=False, validity_end__lte=now) | ||
| .filter(Q(expire_notified__isnull=True) | Q(expire_notified=False)) | ||
| .order_by("validity_end", "pk") | ||
| ) | ||
| expired_ca_ids = {ca.pk for ca in expired_cas} | ||
| renewed_cas = [] | ||
| renewed_certs = [] | ||
| failed_cas = [] | ||
| failed_certs = [] | ||
| manual_expired_cas = [] | ||
| manual_expired_certs = [] | ||
| for ca in expired_cas: | ||
| if ca.can_auto_renew_ca: | ||
| try: | ||
| ca.renew() | ||
| renewed_cas.append(ca) | ||
| except Exception as exc: | ||
| failed_cas.append({"instance": ca, "error": str(exc)}) | ||
| else: | ||
| manual_expired_cas.append(ca) | ||
| renewed_ca_ids = {ca.pk for ca in renewed_cas} | ||
| for cert in expired_certs: | ||
| if cert.ca_id in renewed_ca_ids: | ||
| continue | ||
| if cert.can_auto_renew and cert.ca_id not in expired_ca_ids: | ||
| try: | ||
| cert.renew() | ||
| renewed_certs.append(cert) | ||
| except Exception as exc: | ||
| failed_certs.append({"instance": cert, "error": str(exc)}) | ||
| else: | ||
| manual_expired_certs.append(cert) |
There was a problem hiding this comment.
Process expired objects under an atomic claim.
The task snapshots expired CAs/certs into Python lists and renews them later without any claim/lock step. Two overlapping executions can pick the same rows before either one updates them, so the same CA/cert may be renewed twice, and a mid-ca.renew() exception can leave a partially renewed CA tree while this run still classifies it as failed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@django_x509/tasks.py` around lines 71 - 109, Wrap the selection and renewal
logic in a DB transaction and claim the rows with SELECT ... FOR UPDATE SKIP
LOCKED so concurrent task runs don't process the same objects: replace the two
list(...) queries for expired_cas and expired_certs with queryset iterations
inside a transaction.atomic() block using .select_for_update(skip_locked=True)
(retaining select_related("ca") for expired_certs), then process each locked Ca
and Cert in that transaction (calling ca.renew() and cert.renew()) and update
their state/expire_notified inside the same transaction; also compute
expired_ca_ids/renewed_ca_ids from the locked rows (use ca.pk and cert.ca_id) so
the cert-renew logic (checks against renewed_ca_ids and expired_ca_ids) sees the
claimed/updated state and avoids double-renewal or races.
| When the task runs, it sends a single aggregated email per event type | ||
| using the default Django mail helpers: | ||
|
|
||
| - if ``ADMINS`` is configured, ``mail_admins`` is used | ||
| - if ``MANAGERS`` is configured, ``mail_managers`` is used | ||
| - if neither ``ADMINS`` nor ``MANAGERS`` is configured, all superusers | ||
| with a non-empty email address are notified with ``send_mass_mail`` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify email notification behavior.
The current wording might be slightly misleading. Looking at handlers.py, emails are sent to both ADMINS and MANAGERS if both are configured (not either/or). The superuser fallback only occurs when neither is configured. Consider rewording to:
- if
ADMINSis configured,mail_adminsis used- if
MANAGERSis configured,mail_managersis used- if neither is configured, all superusers with email addresses are notified
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.rst` around lines 385 - 392, Update the README wording to reflect the
actual behavior in handlers.py: state that mail_admins is used when ADMINS is
configured and mail_managers is used when MANAGERS is configured and that both
will be used if both ADMINS and MANAGERS are configured; also clarify that the
superuser fallback (send_mass_mail to superusers with non-empty emails) only
runs when neither ADMINS nor MANAGERS is configured. Mention the exact symbols
mail_admins, mail_managers, ADMINS, MANAGERS, and send_mass_mail so readers can
correlate the docs with handlers.py.
| migrations.AddField( | ||
| model_name="customcert", | ||
| name="expire_notified", | ||
| field=models.BooleanField(default=None, editable=False, null=True), | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the inheritance chain for Customcert model
# Find Customcert model definition
rg -n "class Customcert" --type py -A 5
# Also check if Customcert appears in existing migrations with expire_notified
rg -n "customcert.*expire_notified" --type py -iRepository: openwisp/django-x509
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Find the sample_x509 app directory
find . -type d -name "sample_x509" 2>/dev/null
# List the structure of sample_x509
ls -la tests/openwisp2/sample_x509/ 2>/dev/null || echo "Path not found"
# Check if models.py exists in sample_x509
find . -path "*/sample_x509/models.py" 2>/dev/null
# Read the migration file to understand what models it's referencing
cat -n tests/openwisp2/sample_x509/migrations/0004_ca_auto_renew_ca_expire_notified_and_more.py 2>/dev/null | head -50Repository: openwisp/django-x509
Length of output: 2247
🏁 Script executed:
#!/bin/bash
# Read the actual models.py file in sample_x509
cat -n tests/openwisp2/sample_x509/models.py
# Also check the __init__.py to understand what's imported
cat -n tests/openwisp2/sample_x509/__init__.py
# Check migrations directory to understand the progression
ls -la tests/openwisp2/sample_x509/migrations/ | head -20Repository: openwisp/django-x509
Length of output: 1612
🏁 Script executed:
#!/bin/bash
# Check AbstractCert and AbstractCa definitions in django_x509
rg -n "class AbstractCert|class AbstractCa" --type py -A 3 django_x509/
# Also check if BaseX509 exists and what it contains
rg -n "class BaseX509" --type py -A 5 django_x509/
# Check for expire_notified field definition
rg -n "expire_notified" --type py django_x509/base/models.pyRepository: openwisp/django-x509
Length of output: 1468
Remove redundant expire_notified field additions from this migration.
The expire_notified field is already defined in BaseX509 (line 128 of django_x509/base/models.py) and inherited by all three models through their abstract parents:
Ca→AbstractCa→BaseX509Cert→AbstractCert→BaseX509CustomCert→AbstractCert→BaseX509
The migration should not include the AddField operations for expire_notified on ca, cert, and customcert. Remove all three migrations.AddField calls (lines 27–31, 32–36, 37–41) for the expire_notified field. Only keep the auto_renew field addition for ca.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/openwisp2/sample_x509/migrations/0004_ca_auto_renew_ca_expire_notified_and_more.py`
around lines 37 - 41, Remove the redundant migrations.AddField operations that
add the expire_notified BooleanField to CustomCert, Cert and Ca in the migration
file; expire_notified is already defined on BaseX509 (in
django_x509/base/models.py) and inherited via AbstractCa/AbstractCert into
Ca/Cert/CustomCert, so delete the three migrations.AddField entries that
reference name="expire_notified" (the blocks adding expire_notified for ca, cert
and customcert) and leave only the migrations.AddField that adds the auto_renew
field for Ca.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
django_x509/tasks.py (1)
93-113:⚠️ Potential issue | 🟠 MajorMake CA auto-renew attempts atomic per CA to prevent partial state commits.
If
ca.renew()(Line 96) fails after renewing the CA and some children, those updates can persist while the CA is recorded as failed. This can misclassify downstream cert handling in Line 103-Line 113 and produce inconsistent notification flags.♻️ Proposed fix
for ca in expired_cas: if ca.can_auto_renew_ca: try: - ca.renew(include_revoked_certificates=False) + # isolate each CA renewal in a savepoint: + # either full CA+children renewal succeeds, or all rolls back + with transaction.atomic(): + ca.renew(include_revoked_certificates=False) renewed_cas.append(ca) except Exception as exc: failed_cas.append({"instance": ca, "error": str(exc)})#!/bin/bash # Verify current renewal flow and exception boundaries. rg -n -C4 --type=py 'for ca in expired_cas|ca\.renew\(|failed_cas|with transaction\.atomic|def renew\(self, include_revoked_certificates=True\)|for cert in certificates'Expected verification:
ca.renew()is called inside a broad try/except without a per-CA savepoint, whileAbstractCa.renew()performs multi-step writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@django_x509/tasks.py` around lines 93 - 113, The CA renewal loop is not atomic per CA and can leave partial writes if ca.renew() fails; wrap each CA renewal in its own database transaction/savepoint (e.g., using transaction.atomic() or savepoint_rollback) around ca.renew() so that any exception rolls back that CA's changes, only append to renewed_cas after the transaction successfully commits, and move failed entries into failed_cas on exception; ensure renewed_ca_ids is computed from only successfully committed CAs so the subsequent expired_certs handling (expired_certs, cert.can_auto_renew, renewed_ca_ids, manual_expired_certs, failed_certs) sees consistent state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@django_x509/tasks.py`:
- Around line 128-140: The signal dispatch of x509_objects_expired and the
subsequent calls to _mark_expired_objects_notified are currently executed inside
the with transaction.atomic() that holds select_for_update locks; move the
dispatch and the two _mark_expired_objects_notified calls (which trigger
notify_x509_objects_expired → mail_admins external I/O) to after the
transaction.atomic block completes so the signal handlers run outside the DB
transaction and do not hold locks or risk IO failures aborting the transaction,
while still returning the same result dict; ensure the code uses the same local
variables (manual_expired_cas, manual_expired_certs, renewed_cas, renewed_certs,
failed_cas, failed_certs) when calling x509_objects_expired and
_mark_expired_objects_notified.
In `@django_x509/tests/test_tasks.py`:
- Around line 106-177: Add a new test method (e.g.
test_check_x509_expiration_ca_partial_renew_failure_rolls_back) that creates a
superuser, creates a CA with auto_renew=AutoRenewChoices.CA_AND_CERTIFICATES and
two child certs (e.g. "good-child-cert" and "bad-child-cert"), set CA and both
certs validity_end to past (timedelta(days=-1)), record old CA.validity_end and
both certs' serials/certificates, patch the bad cert instance's renew method to
raise Exception("boom") (patch.object(bad_cert, "renew",
side_effect=Exception("boom"))), call check_x509_expiration(), refresh CA and
certs from DB and assert that CA.validity_end was NOT updated (equal to old
value) and neither child's serial/certificate changed (no partial renewal
persisted), and assert an email was sent reporting the failure; name the test to
match existing style and reuse helpers _create_ca, _create_cert and
_set_validity_end to locate code paths.
In `@README.rst`:
- Line 397: Replace the nonstandard spelling "overrideable" with the standard
"overridable" in the README text that currently reads "The task emits two
overrideable Django signals:" (search for the exact phrase "overrideable Django
signals" or the word "overrideable" to locate the occurrence).
---
Duplicate comments:
In `@django_x509/tasks.py`:
- Around line 93-113: The CA renewal loop is not atomic per CA and can leave
partial writes if ca.renew() fails; wrap each CA renewal in its own database
transaction/savepoint (e.g., using transaction.atomic() or savepoint_rollback)
around ca.renew() so that any exception rolls back that CA's changes, only
append to renewed_cas after the transaction successfully commits, and move
failed entries into failed_cas on exception; ensure renewed_ca_ids is computed
from only successfully committed CAs so the subsequent expired_certs handling
(expired_certs, cert.can_auto_renew, renewed_ca_ids, manual_expired_certs,
failed_certs) sees consistent state.
🪄 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: eaec0e38-5f2d-4f2f-97f3-c985bf6d769c
📒 Files selected for processing (4)
README.rstdjango_x509/base/models.pydjango_x509/tasks.pydjango_x509/tests/test_tasks.py
📜 Review details
🧰 Additional context used
🧠 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_tasks.pydjango_x509/tasks.pydjango_x509/base/models.py
🔇 Additional comments (4)
README.rst (1)
385-392: Notification recipient precedence is now clearly documented.This section now correctly states that both
ADMINSandMANAGERScan be used together, with superuser fallback only when neither is configured.django_x509/base/models.py (2)
128-128:expire_notifiedlifecycle handling is consistent with renewal flow.Resetting
expire_notifiedduring renew keeps future expiration notifications eligible after a successful reissue.Also applies to: 677-680
729-742: Good safeguard: CA renewal can exclude revoked child certificates.The new
include_revoked_certificatesbranch provides the control needed to avoid unintended reissuance of revoked certs during CA auto-renew.django_x509/tests/test_tasks.py (1)
29-252: Great test coverage for the new expiration task behavior.The suite exercises recipient routing, retry behavior, signal overrides, auto-renew branches, and revoked-child handling comprehensively.
| notified = _get_signal_success( | ||
| x509_objects_expired, | ||
| expired_cas=manual_expired_cas, | ||
| expired_certs=manual_expired_certs, | ||
| renewed_cas=renewed_cas, | ||
| renewed_certs=renewed_certs, | ||
| failed_cas=failed_cas, | ||
| failed_certs=failed_certs, | ||
| ) | ||
| if notified: | ||
| _mark_expired_objects_notified(manual_expired_cas) | ||
| _mark_expired_objects_notified(manual_expired_certs) | ||
| return { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify signal dispatch location and that default handlers perform email sending.
rg -n -C3 --type=py 'with transaction\.atomic|_get_signal_success\(|x509_objects_expired|notify_x509_objects_expired|_send_notification_email'Repository: openwisp/django-x509
Length of output: 6450
🏁 Script executed:
cat -n django_x509/tasks.py | sed -n '40,150p'Repository: openwisp/django-x509
Length of output: 4938
Move signal dispatch and notifications outside database transaction.
Line 128 dispatches x509_objects_expired inside the with transaction.atomic() block opened at line 72. The default handler notify_x509_objects_expired calls mail_admins() to send email notifications, performing external I/O while holding row locks acquired by select_for_update(). This increases database contention and amplifies the impact of email delivery failures on the transaction.
Suggested refactor
- with transaction.atomic():
+ with transaction.atomic():
...
- notified = _get_signal_success(
- x509_objects_expired,
- ...
- )
- if notified:
- _mark_expired_objects_notified(manual_expired_cas)
- _mark_expired_objects_notified(manual_expired_certs)
+ # Signal and notifications dispatched after transaction commits
+ notified = _get_signal_success(
+ x509_objects_expired,
+ ...
+ )
+ if notified:
+ with transaction.atomic():
+ _mark_expired_objects_notified(manual_expired_cas)
+ _mark_expired_objects_notified(manual_expired_certs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@django_x509/tasks.py` around lines 128 - 140, The signal dispatch of
x509_objects_expired and the subsequent calls to _mark_expired_objects_notified
are currently executed inside the with transaction.atomic() that holds
select_for_update locks; move the dispatch and the two
_mark_expired_objects_notified calls (which trigger notify_x509_objects_expired
→ mail_admins external I/O) to after the transaction.atomic block completes so
the signal handlers run outside the DB transaction and do not hold locks or risk
IO failures aborting the transaction, while still returning the same result
dict; ensure the code uses the same local variables (manual_expired_cas,
manual_expired_certs, renewed_cas, renewed_certs, failed_cas, failed_certs) when
calling x509_objects_expired and _mark_expired_objects_notified.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@django_x509/tests/test_tasks.py`:
- Around line 246-255: The test disconnects the original signal handler
incorrectly; replace
x509_objects_expiring.disconnect(notify_x509_objects_expiring) with a disconnect
call that uses the same dispatch_uid used in apps.py
(dispatch_uid="django_x509.notify_x509_objects_expiring") so the original
notify_x509_objects_expiring is actually removed before connecting the test
receiver, then restore the original by reconnecting with that same dispatch_uid
after check_x509_expiration() (referencing notify_x509_objects_expiring,
x509_objects_expiring.disconnect/connect, dispatch_uid, and
test_expiring_receiver).
- Around line 167-168: The test has an inconsistent type comparison:
revoked_cert.serial_number is compared to str(old_revoked_serial) while
active_cert.serial_number is compared directly to old_active_serial; make these
consistent by removing the unnecessary str() wrapper or by casting both sides
the same way. Update the assertion in test_tasks.py to use
self.assertEqual(revoked_cert.serial_number, old_revoked_serial) (or
alternatively cast both sides e.g., int(revoked_cert.serial_number) ==
int(old_revoked_serial)) so that active_cert.serial_number,
revoked_cert.serial_number, old_active_serial and old_revoked_serial are
compared using the same type.
- Around line 305-314: The disconnect/connect sequence in the test uses
x509_objects_expired.disconnect(notify_x509_objects_expired) without the
dispatch_uid, so the original handler (notify_x509_objects_expired) may not be
properly removed; update the initial disconnect to use the same dispatch_uid
used when the handler was originally connected (i.e., pass
dispatch_uid="django_x509.notify_x509_objects_expired") before connecting the
temporary receiver, ensure the final reconnect uses the same dispatch_uid as
shown, and keep the rest of the try/finally flow around check_x509_expiration()
intact (symbols: x509_objects_expired, notify_x509_objects_expired, receiver,
check_x509_expiration).
- Around line 172-220: The test names ending with "_is_retried" are misleading
because check_x509_expiration catches exceptions and does not use Celery retry
semantics; rename the two tests (currently
test_check_x509_expiration_failed_auto_renew_is_retried and
test_check_x509_expiration_failed_ca_auto_renew_is_retried) to reflect that a
subsequent manual invocation can succeed (e.g.,
test_check_x509_expiration_failed_auto_renew_succeeds_on_next_run and
test_check_x509_expiration_failed_ca_auto_renew_succeeds_on_next_run), and
update any test docstrings/comments to mention check_x509_expiration manual
re-run behavior rather than automatic retrying; ensure any references to the old
names (imports or test selectors) are updated accordingly.
- Around line 282-291: The disconnect call in the finally block uses only
dispatch_uid and won't remove the test receiver; change it to call
x509_objects_expired.disconnect(receiver, dispatch_uid="test_expired_receiver")
so the disconnect matches the connect call, and when re-connecting the original
handler ensure you pass the same dispatch_uid used originally
("django_x509.notify_x509_objects_expired") to
x509_objects_expired.connect(notify_x509_objects_expired,
dispatch_uid="django_x509.notify_x509_objects_expired").
🪄 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: c87f5d50-4baf-43fb-9729-47a0768d4cb5
📒 Files selected for processing (1)
django_x509/tests/test_tasks.py
📜 Review details
🧰 Additional context used
🧠 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_tasks.py
🔇 Additional comments (4)
django_x509/tests/test_tasks.py (4)
1-27: LGTM!The imports are appropriate, and the
_set_validity_endhelper method is well-designed for reusability across tests. The class-level@override_settingsfixture properly configures the expiration notice period.
29-62: LGTM!These tests correctly verify the recipient resolution priority: ADMINS/MANAGERS take precedence, and superusers serve as fallback when neither is configured. The assertions properly validate that
expire_notifiedremainsNonefor expiring (not yet expired) certificates.
64-102: LGTM!Good coverage of the
expire_notifiedflag behavior. The tests verify both the initial notification and the idempotency guarantee (no duplicate notifications on subsequent task runs).
316-318: LGTM!Good assertion coverage—verifying that an exception in the signal receiver prevents
expire_notifiedfrom being set, ensuring the notification can be retried on the next task run.
0149d5b to
98647bd
Compare
31c4ba7 to
6c14e9b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
django_x509/tasks.py (1)
130-142:⚠️ Potential issue | 🟠 MajorMove signal dispatch and notifications outside database transaction.
The signal dispatch (
x509_objects_expired) and the_mark_expired_objects_notifiedcalls are inside thetransaction.atomic()block (started at line 72). The default handlernotify_x509_objects_expiredsends emails, performing external I/O while holding row locks acquired byselect_for_update(). This increases database contention and risks transaction rollback on email delivery failures.,
♻️ Suggested refactor
- with transaction.atomic(): ... + # Collect data needed for signal dispatch if not any([...]): return {...} - notified = _get_signal_success( - x509_objects_expired, - ... - ) - if notified: - _mark_expired_objects_notified(manual_expired_cas) - _mark_expired_objects_notified(manual_expired_certs) + # Signal and notifications dispatched after transaction commits + notified = _get_signal_success( + x509_objects_expired, + expired_cas=manual_expired_cas, + expired_certs=manual_expired_certs, + renewed_cas=renewed_cas, + renewed_certs=renewed_certs, + failed_cas=failed_cas, + failed_certs=failed_certs, + ) + if notified: + with transaction.atomic(): + _mark_expired_objects_notified(manual_expired_cas) + _mark_expired_objects_notified(manual_expired_certs) return {...}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@django_x509/tasks.py` around lines 130 - 142, The signal dispatch and notification marking are inside a DB transaction and must be moved outside it: stop calling x509_objects_expired and _mark_expired_objects_notified while the transaction.atomic() (and any select_for_update() locks) is active; instead, collect manual_expired_cas/manual_expired_certs/renewed_cas/renewed_certs/failed_cas/failed_certs inside the transaction, commit, then after the transaction completes call x509_objects_expired(...) and, if it returns truthy, call _mark_expired_objects_notified(manual_expired_cas) and _mark_expired_objects_notified(manual_expired_certs); ensure notify_x509_objects_expired (signal handlers that send email) run only after commit to avoid I/O or external failures occurring while holding DB locks.README.rst (1)
385-393: 🧹 Nitpick | 🔵 TrivialClarify email notification behavior.
The current wording might be slightly misleading. Based on the handler code pattern, when both
ADMINSandMANAGERSare configured, bothmail_adminsandmail_managersare called (not either/or). The superuser fallback only triggers when neither is configured.Consider rewording to make it clearer that both helpers are used when both settings are present:
- if
ADMINSis configured,mail_adminsis used- if
MANAGERSis configured,mail_managersis used- if neither is configured, all superusers with email addresses are notified
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.rst` around lines 385 - 393, The README wording about email notifications is ambiguous: update the paragraph to clearly state the handler behavior using the exact symbols ADMINS, MANAGERS, mail_admins, mail_managers and the superuser fallback; explicitly say that mail_admins is called when ADMINS is configured, mail_managers is called when MANAGERS is configured, both are called when both are present, and send_mass_mail to superusers is only used when neither ADMINS nor MANAGERS is configured. Keep the bullet list concise and replace the current three-line bullets with the clarified sequence using those exact identifiers.django_x509/tests/test_tasks.py (1)
148-170: 🧹 Nitpick | 🔵 TrivialInconsistent type handling for
serial_numbercomparison.Lines 159-160 have inconsistent treatment:
old_active_serialis stored as-is whileold_revoked_serialis wrapped instr(). Then at line 168, the comparison usesold_revoked_serial(already a string) directly. This inconsistency is confusing—if both serials are the same type from the model, handle them consistently.♻️ Suggested fix for consistency
old_active_serial = active_cert.serial_number - old_revoked_serial = str(revoked_cert.serial_number) + old_revoked_serial = revoked_cert.serial_number old_revoked_certificate = revoked_cert.certificate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@django_x509/tests/test_tasks.py` around lines 148 - 170, The test test_check_x509_expiration_auto_renew_ca_skips_revoked_certificates mixes types when capturing serials; make the captured serials consistent by converting both old_active_serial and old_revoked_serial to the same type (e.g., wrap active_cert.serial_number and revoked_cert.serial_number in str()) so the subsequent assertions compare like-for-like (refer to variables old_active_serial, old_revoked_serial and revoked_cert.serial_number in the test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@django_x509/tasks.py`:
- Around line 130-142: The signal dispatch and notification marking are inside a
DB transaction and must be moved outside it: stop calling x509_objects_expired
and _mark_expired_objects_notified while the transaction.atomic() (and any
select_for_update() locks) is active; instead, collect
manual_expired_cas/manual_expired_certs/renewed_cas/renewed_certs/failed_cas/failed_certs
inside the transaction, commit, then after the transaction completes call
x509_objects_expired(...) and, if it returns truthy, call
_mark_expired_objects_notified(manual_expired_cas) and
_mark_expired_objects_notified(manual_expired_certs); ensure
notify_x509_objects_expired (signal handlers that send email) run only after
commit to avoid I/O or external failures occurring while holding DB locks.
In `@django_x509/tests/test_tasks.py`:
- Around line 148-170: The test
test_check_x509_expiration_auto_renew_ca_skips_revoked_certificates mixes types
when capturing serials; make the captured serials consistent by converting both
old_active_serial and old_revoked_serial to the same type (e.g., wrap
active_cert.serial_number and revoked_cert.serial_number in str()) so the
subsequent assertions compare like-for-like (refer to variables
old_active_serial, old_revoked_serial and revoked_cert.serial_number in the
test).
In `@README.rst`:
- Around line 385-393: The README wording about email notifications is
ambiguous: update the paragraph to clearly state the handler behavior using the
exact symbols ADMINS, MANAGERS, mail_admins, mail_managers and the superuser
fallback; explicitly say that mail_admins is called when ADMINS is configured,
mail_managers is called when MANAGERS is configured, both are called when both
are present, and send_mass_mail to superusers is only used when neither ADMINS
nor MANAGERS is configured. Keep the bullet list concise and replace the current
three-line bullets with the clarified sequence using those exact identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e27f417-5968-4c8d-8b5b-13f246b7c36d
📒 Files selected for processing (3)
README.rstdjango_x509/tasks.pydjango_x509/tests/test_tasks.py
📜 Review details
🧰 Additional context used
🧠 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_tasks.pydjango_x509/tasks.py
🔇 Additional comments (22)
README.rst (2)
59-61: LGTM!The feature descriptions accurately reflect the implementation: daily Celery task for expiration checks and configurable CA auto-renewal modes.
394-409: LGTM!The signals documentation clearly describes the two signals with their signatures and explains how to override the default receivers.
django_x509/tasks.py (9)
1-11: LGTM!The imports are appropriate for the task's requirements: Celery, Django transactions, timezone utilities, and the required signals.
13-19: LGTM!The
_get_day_windowhelper correctly computes a timezone-aware day window for the expiration notice check.
22-29: LGTM!The
_get_signal_successhelper properly handles signal dispatch usingsend_robust, aggregates success from all receivers, and correctly returnsFalseon any exception.
32-38: LGTM!The bulk update helper efficiently marks instances as notified using a single query.
46-71: LGTM!The expiring objects logic correctly:
- Checks for
EXPIRATION_NOTICE_DAYS >= 0before processing- Filters for non-revoked certificates
- Excludes already-expired objects with
validity_end__gt=now- Emits the expiring signal with proper parameters
72-85: LGTM!The expired objects query correctly uses
select_for_update()to prevent concurrent task runs from processing the same objects. The filter appropriately excludes already-notified items.
93-102: LGTM!The CA renewal logic correctly:
- Checks
can_auto_renew_cabefore attempting renewal- Wraps renewal in a nested
transaction.atomic()for isolation- Passes
include_revoked_certificates=Falseto skip revoked certs- Catches exceptions and records failures without marking as notified
104-115: LGTM!The certificate renewal logic correctly:
- Skips certificates already renewed via CA renewal (
cert.ca_id in renewed_ca_ids)- Verifies the CA isn't expired (
cert.ca_id not in expired_ca_ids)- Checks
cert.can_auto_renewbefore attempting renewal- Uses nested atomic transaction for isolation
142-152: LGTM!The return dictionary provides comprehensive observability with counts for all categories.
django_x509/tests/test_tasks.py (11)
1-19: LGTM!Imports are appropriate for the test suite, including the required handlers, signals, and task under test.
22-27: LGTM!The test class setup with
@override_settingsforDJANGO_X509_EXPIRATION_NOTICE_DAYS=3and the_set_validity_endhelper are well designed for controlling test scenarios.
29-44: LGTM!Good test coverage for the superuser fallback email recipient behavior. The assertions verify correct email delivery, content, and that
expire_notifiedremainsNonefor expiring (not yet expired) certificates.
64-103: LGTM!Excellent tests for the
expire_notifiedflag behavior:
- Verifies expired objects are marked as notified after email sent
- Confirms subsequent runs don't re-send notifications
104-146: LGTM!Comprehensive auto-renewal tests covering:
- Certificate auto-renewal with
CERTIFICATES_ONLYmode- CA auto-renewal with
CA_AND_CERTIFICATESmode- Verification of updated validity dates and serial numbers
- Confirmation that
expire_notifiedis cleared after renewal
172-222: LGTM!Well-named tests (
*_is_attempted_on_next_run) clearly describe the behavior being verified. The tests correctly validate that failed renewals:
- Don't set
expire_notified(allowing future retries)- Report failures in email
- Succeed on subsequent task invocations
224-268: LGTM!Excellent regression test for CA partial-renew failure rollback. The test verifies that when a child certificate renewal fails during CA auto-renew:
- The CA is not partially renewed
- No child certificates are renewed
- All original serial numbers and certificates are preserved
- Failure is properly reported
282-310: LGTM!The signal override test correctly uses
dispatch_uidfor both disconnect and connect operations, properly restoring the original handler in thefinallyblock.
312-348: LGTM!Same proper pattern for the expired signal override test with correct
dispatch_uidhandling.
350-373: LGTM!Good test for exception handling in signal receivers - verifies that when a receiver raises an exception:
notifiedreturnsFalseexpire_notifiedremainsNone(allowing retry)- No emails are sent
398-432: LGTM!Good edge case test that verifies the expiring notification email correctly includes sections for:
- Manual CAs
- Auto-renewable CAs
- Auto-renewable certificates
The patch on
reversesimulates URL resolution failure and verifies "unavailable" fallback text.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
First, thank you @czarflix, the quality here is clearly above average: select_for_update inside an atomic block, the expire_notified flag to avoid duplicate notices, overridable signals, tests and docs. This is real engineering, not a drive-by.
That said, I want to step back on scope before we go line by line, because I think part of this belongs somewhere else, and that decision changes the rest of the PR.
Main concern: scope and where this logic lives
django-x509 is intentionally a minimal, low-level certificate library. This PR pulls a fair amount of higher-level product behavior into it:
- A Celery task plus beat schedule (and a new
openwisp-utils[celery]dependency). - A bespoke email-dispatch path (
mail_admins/mail_managers/ fallback to superusers viasend_mass_mail). - Automatic renewal of CAs and certificates.
In the OpenWISP architecture, periodic checks and user-facing notifications normally live in the higher-level apps (openwisp-controller) and go through openwisp-notifications, not through a hand-rolled email path inside a base library. Reinventing notification delivery here means it will not respect a user's notification preferences, channels, or organization scoping, and it duplicates infrastructure we already have.
My strong preference:
- Keep the genuinely lightweight, library-appropriate parts here: the
is_expiredproperty and theExpiredcolumn in the admin changelist. Those are great and belong in django-x509. - Move the periodic task, the notification dispatch, and especially auto-renewal into openwisp-controller (or expose just the primitives here and let controller orchestrate them via openwisp-notifications and the existing Celery setup).
Let's agree on this split first, because it determines how much of the rest survives.
Auto-renewal needs a serious rethink (security/operational)
renew() does not extend validity in place: it regenerates the serial and calls _generate(), which creates a brand new private key. So "auto renew" means automatic re-keying. For a CA, renew(include_revoked_certificates=False) re-keys the CA and re-keys every non-revoked issued certificate, automatically, from a daily cron task.
That is a very high-impact action to trigger silently:
- A new CA key invalidates trust for every relying party until the new CA certificate is distributed.
- Every renewed leaf certificate gets a new key/cert that must be redistributed to the device using it.
Doing this unattended is dangerous. At minimum it needs very loud documentation, but I would argue auto-renewal of a CA should not happen automatically at all, and certificate auto-renewal should only exist where the higher-level app can coordinate redistribution (again pointing to controller). Please also make explicit in the docs/UI that renewal re-keys; users may expect same-key renewal.
Concrete robustness issues
-
Missed "expiring soon" notices. The expiring query uses a single one-day window exactly
EXPIRATION_NOTICE_DAYSfrom today, with no persistent flag likeexpire_notified. If the worker misses a single daily run (deploy, outage), the entire advance warning for everything expiring that day is lost permanently. The "expired" path is protected byexpire_notified, the "expiring" path is not. Consider notifying for the wholevalidity_end <= now + notice_daysrange with a persistent "expiring notified" flag, so a missed run self-heals. -
notifiedflag depends on receivers returning truthy._get_signal_successonly treats the run as notified if some receiver returns a truthy value. So_mark_expired_objects_notifiedis gated on the receiver contract. If a custom receiver (or the default one) forgets to returnTrue, expired objects are never marked and get re-notified every single day forever; conversely a too-eager return marks them prematurely. This implicit contract is fragile, document it clearly and make the default receiver's return value explicit and tested.
To be clear, none of this is a knock on the implementation craft. It is about where this functionality should live and how risky unattended re-keying is. Let's settle the scope/split question and I will happily review the trimmed-down version in detail.
Conflicting Migrations DetectedHello @czarflix and @nemesifier, The CI failed due to conflicting Django migrations in the Fix: python manage.py makemigrations --mergeAfter running this command, Django will prompt you to resolve the conflicts. Once resolved, commit the newly generated migration file(s) and push again. |
3 participants
Checklist
Reference to Existing Issue
Closes #202.
Description of Changes
This adds expiration handling to django-x509. The root issue was that certificates and CAs exposed validity_end, but the app did not provide an expiration workflow: there was no expired-status indicator in the admin, no periodic expiration check, no default notification path, and no automatic renewal policy for expired objects.
This change adds an Expired column to the certificate admin changelist, introduces a daily expiration check with configurable advance notices and overridable Django signals, adds expire_notified so expired notifications are not lost, and adds CA automatic renewal modes for certificates or for both CAs and certificates. The README, changelog, and migrations have been updated accordingly.
Manual validation performed:
Screenshot