[feature:ui] Make x509 extensions more flexible and user friendly#214
[feature:ui] Make x509 extensions more flexible and user friendly#214czarflix wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds JSON Schema–backed validation and a browser-based admin editor for X.509 extensions. Introduces ExtensionsWidget (JS/CSS/template), a schemas.py module with default CA and certificate schemas and settings hooks, model-level normalization and validation using jsonschema (new helper methods and constants and updated clean_fields signature), admin integration to inject the widget and schema, extensive tests for schema behavior, README/CHANGES documentation updates, and adds jsonschema to install_requires. No exported API removals. Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as ExtensionsWidget (JS)
participant AdminForm as Django Admin Form
participant Model as BaseX509 Model
participant SchemaSvc as jsonschema Validator
User->>Browser: Open admin form / edit extensions
Browser->>Browser: Parse data-schema, render UI
User->>Browser: Add/Edit extension rows
Browser->>Browser: Normalize items, update hidden textarea (JSON)
User->>AdminForm: Submit form
AdminForm->>Model: full_clean()/clean_fields()
Model->>SchemaSvc: validate extensions against schema
alt validation passes
SchemaSvc-->>Model: validated
Model->>Model: normalize values & DER-encode extensions
Model-->>AdminForm: accept cleaned data
AdminForm-->>User: save successful
else validation fails
SchemaSvc-->>Model: errors
Model->>AdminForm: raise ValidationError (path-aware)
AdminForm-->>User: display errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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: 3
🤖 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/base/models.py`:
- Around line 763-765: Replace the duplicated parsing logic in _add_extensions
with a call to the shared helper _get_extension_values: instead of doing values
= val if isinstance(val, list) else val.split(",") and iterating to strip/lower
each entry, call _get_extension_values(val) to obtain the normalized list and
iterate that; update both occurrences (the block using variables values/v_clean
and the similar block around the other duplicate) to use _get_extension_values
so the behavior is consistent and more defensive.
- Around line 626-630: The nsComment branch currently checks len(value)
(characters) which fails for multi-byte characters; before DER encoding you must
validate the byte-length by encoding the value to UTF-8 (or the same encoding
used for DER, e.g. value_bytes = value.encode('utf-8')) and ensure
len(value_bytes) <= 255, and if it exceeds call
self._raise_extensions_validation_error with the same path and message; update
the validation in the nsComment handling so the byte-length check replaces the
character-length check and occurs prior to any DER encoding or payload
construction.
In `@django_x509/schemas.py`:
- Line 19: Remove "critical" from the JSON Schema required array so payloads
that omit critical remain valid; specifically update the "required": ["name",
"critical", "value"] entry in schemas.py to exclude "critical" (e.g. ["name",
"value"]) and rely on the existing extension-handling logic that defaults
missing critical to False. Ensure no other schema consumers expect critical to
be present and run tests for extension parsing to confirm backward-compatible
behavior.
🪄 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: 809ddf06-79da-4c09-aea8-c1729a6006c1
📒 Files selected for processing (15)
CHANGES.rstREADME.rstdjango_x509/base/admin.pydjango_x509/base/models.pydjango_x509/base/widgets.pydjango_x509/schemas.pydjango_x509/settings.pydjango_x509/static/django-x509/css/extensions-widget.cssdjango_x509/static/django-x509/js/extensions-widget.jsdjango_x509/templates/django_x509/widgets/extensions.htmldjango_x509/tests/test_admin.pydjango_x509/tests/test_ca.pydjango_x509/tests/test_cert.pydjango_x509/tests/test_extensions.pysetup.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/tests/test_ca.pydjango_x509/tests/test_cert.pydjango_x509/settings.pydjango_x509/base/widgets.pydjango_x509/base/models.pydjango_x509/tests/test_admin.pydjango_x509/tests/test_extensions.pysetup.pydjango_x509/schemas.pydjango_x509/base/admin.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
🪛 HTMLHint (1.9.2)
django_x509/templates/django_x509/widgets/extensions.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (14)
django_x509/tests/test_cert.py (1)
325-325: Field-level error assertion is correctly targeted.Line 325 and Line 334 now assert against the
extensionskey inmessage_dict, which is the right contract for structured field validation errors.Also applies to: 334-334
CHANGES.rst (1)
7-11: Changelog entry is clear and release-appropriate.The new 1.4.0 entry communicates the extension editor and schema-validation feature accurately.
django_x509/base/models.py (1)
179-188:clean_fields(exclude=...)integration is correct.Line 186 and Line 187 properly preserve Django exclusion semantics while still validating
extensionswhen included.django_x509/schemas.py (1)
119-144: Schema retrieval and validation flow is well-structured.The validate-then-return pattern with deep-copying prevents shared-state mutation and surfaces bad overrides early.
django_x509/settings.py (1)
28-33: Settings passthrough functions are clean and appropriate.Line 28 and Line 32 keep schema resolution centralized without duplicating schema logic in callers.
django_x509/tests/test_ca.py (1)
306-306: Validation assertion is now aligned with structured errors.Line 306 and Line 315 correctly assert presence of the
extensionsfield key.Also applies to: 315-315
django_x509/base/admin.py (1)
84-90: Dynamic schema injection into the widget is correctly implemented.Line 88 wires the widget at form-build time, and Line 149/Line 242 cleanly separate CA vs certificate schema sources.
Also applies to: 149-151, 242-243
django_x509/static/django-x509/css/extensions-widget.css (1)
1-88: LGTM!The CSS is well-structured with BEM-like naming conventions, appropriate responsive breakpoints, and clean layout styles. The grid-based choice list and responsive adjustments for narrow viewports are sensible design choices.
django_x509/templates/django_x509/widgets/extensions.html (1)
1-16: LGTM!The template correctly uses context variables
schemaandtextareathat match the keys defined inExtensionsWidget.render(). The data attributes for i18n labels are properly structured for the JavaScript widget to consume via the dataset API.Note: The static analysis hint about missing doctype is a false positive—this is a template fragment intended to be embedded within a complete HTML document, not a standalone page.
django_x509/base/widgets.py (1)
1-32: LGTM!The widget implementation is clean and follows Django patterns. The CSS class handling is defensive, the media property correctly declares dependencies, and the render method properly passes the JSON-encoded schema to the template.
README.rst (1)
332-374: LGTM!The documentation clearly explains the new schema settings, their defaults, and the fallback behavior for unsupported schemas. The note about legacy comma-separated value normalization is helpful for backward compatibility awareness.
django_x509/static/django-x509/js/extensions-widget.js (1)
1-314: LGTM!The JavaScript implementation is well-structured with clear separation of concerns:
- Schema parsing with proper validation of supported value types
- Item normalization including legacy comma-separated value handling
- Clean DOM building with appropriate event handlers
- Proper error handling with user-friendly messages
The full re-render approach on changes is acceptable for this admin widget use case where extension lists are typically small.
django_x509/tests/test_admin.py (1)
160-216: LGTM!The new tests provide good coverage for the extensions widget:
- Asset loading verification (JS/CSS)
- Schema content verification (CA vs Cert schemas have different extensions)
- Settings override behavior
- Proper test isolation with
try/finallyforreadonly_fieldsThe regex-based extraction of
data-schemawith JSON parsing is a pragmatic approach for verifying the schema content in rendered output.django_x509/tests/test_extensions.py (1)
1-142: LGTM!Excellent test coverage for the extensions schema validation:
- Default schema enforcement (CA vs Cert extensions)
- List value validation with actual x509 certificate verification
- Invalid payload rejection
- Legacy comma-separated value normalization (backward compatibility)
- Settings override behavior
- Security guardrails that reject unsupported extension names and incompatible value types even when schema is overridden
The use of
deepcopyfor schema modifications and nested context managers for settings overrides demonstrates good test isolation practices.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/base/models.py`:
- Around line 604-607: The _get_der_length_bytes helper currently only supports
short form and one-byte long form and will fail for lengths >= 256; update
_get_der_length_bytes to implement full DER length encoding: if length < 128
return single byte, otherwise compute the minimal big-endian byte sequence for
length (no leading zero bytes), prepend a length-of-length byte with 0x80 |
num_bytes, and return that sequence; also validate length is non-negative (raise
ValueError for negatives) so the function is robust if nsComment's 255 cap
changes.
In `@django_x509/schemas.py`:
- Around line 45-50: The JSON schema for the nsComment field (and the other
identical occurrence) currently uses "maxLength": 255 which counts characters
but your model method _validate_supported_extensions enforces a 255-byte UTF‑8
limit; update the schema entries referenced in the diff to either (preferred)
set a conservative "maxLength": 85 (to account for worst‑case 3‑byte UTF‑8
characters) and add a "description" explaining the model enforces a 255‑byte
limit, or (alternatively) keep the char limit but add a clear "description"
stating that actual validation is by bytes in _validate_supported_extensions;
apply the same change to the second block noted in the comment (lines 77-82).
In `@django_x509/tests/test_admin.py`:
- Around line 178-218: Both tests manipulate class-level state by copying and
restoring cert_admin.readonly_fields in a try/finally; instead, avoid that
pattern by keeping tests isolated — either instantiate a fresh admin instance
and avoid touching cert_admin.readonly_fields, or temporarily patch the
attribute with unittest.mock.patch.object (or use self.addCleanup) around the
call that builds the form (look at test_cert_extensions_widget and
test_cert_extensions_widget_uses_overridden_schema, where ma =
self.cert_admin(Cert, self.site) and widget =
ma.get_form(request).base_fields["extensions"].widget are used); remove the
try/finally backup/restore and use patching or fresh instantiation so
class-level readonly_fields is not mutated across tests.
🪄 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: ae6a38a4-812f-4691-96f0-0761ad1361b0
📒 Files selected for processing (4)
django_x509/base/models.pydjango_x509/schemas.pydjango_x509/tests/test_admin.pydjango_x509/tests/test_extensions.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/schemas.pydjango_x509/base/models.pydjango_x509/tests/test_admin.pydjango_x509/tests/test_extensions.py
🔇 Additional comments (24)
django_x509/tests/test_admin.py (3)
1-15: LGTM!The new imports are appropriate for the added test functionality:
jsonandrefor parsing widget output,unescapefor HTML entity decoding,override_settingsfor testing schema customization, andBaseAdminfor testing the default extensions schema behavior.
162-176: LGTM!Comprehensive test for CA extensions widget rendering. Properly verifies:
- Static assets (JS/CSS) inclusion
- Widget CSS class presence
- Schema data attribute with correct title
- Correct extensions availability (nsComment present, extendedKeyUsage absent for CA)
257-268: LGTM!Good additions:
test_revoke_action_multipleproperly tests the plural message casetest_base_admin_default_extensions_schemaverifies the base class returns an empty schema, ensuring subclasses must provide their owndjango_x509/base/models.py (8)
42-57: LGTM!Well-defined constant sets for supported extension values. Using sets provides O(1) lookup for validation checks.
179-188: LGTM!The
clean_fieldssignature update correctly propagates theexcludeparameter to the parent class and respects it when validating extensions. This follows Django's field validation conventions.
528-557: LGTM!The
_get_extensions_schemaand_normalize_extensionsmethods properly:
- Distinguish between CA and Cert based on the presence of
caattribute- Handle
Noneextensions by converting to empty list- Normalize comma-separated string values to lists based on schema expectations
559-595: LGTM!The error handling methods provide a clean abstraction:
_get_best_extensions_erroruses jsonschema's best_match for user-friendly error selection_format_extensions_errorbuilds readable path notation for nested errors_raise_extensions_validation_errorprovides consistent error formatting
609-681: LGTM!The
_validate_supported_extensionsmethod provides thorough validation:
- Validates
criticalflag is boolean- Type-checks nsComment value and enforces byte-length limit (addressing past review)
- Validates extendedKeyUsage and nsCertType against allowed value sets
- Properly handles both list and string value formats
- Raises descriptive errors with precise paths
683-692: LGTM!The
_validate_extensionsmethod correctly orchestrates the validation pipeline:
- Gets the appropriate schema (CA vs Cert)
- Normalizes extensions
- Validates against JSON schema
- Runs additional supported-extension validation
769-780: LGTM!The
_add_extensionsmethod now correctly uses_get_extension_valueshelper (addressing past review comment) for bothextendedKeyUsageandnsCertTypeprocessing, ensuring consistent parsing behavior.Also applies to: 813-820
787-794: LGTM!The nsComment DER encoding now correctly:
- Encodes value to UTF-8 bytes first
- Validates byte length (addressing past review)
- Uses
_get_der_length_bytesfor proper DER length encoding (supporting long form for 128-255 bytes)django_x509/schemas.py (6)
8-20: LGTM!The
_build_extension_schemahelper is well-designed:
- Enforces strict object shape with
additionalProperties: False- Sets sensible default for
criticalflag- Correctly excludes
criticalfrom required fields (addressing past review comment), maintaining backward compatibility
23-31: LGTM!The
_build_multi_value_schemahelper properly defines array-based value schemas with:
- String item type with enum constraints
- Minimum one item requirement
- Unique items constraint to prevent duplicates
34-64: LGTM!The CA extensions schema appropriately limits nsCertType to CA-oriented values (
sslca,emailca,objca) and excludesextendedKeyUsagewhich is certificate-specific.
66-106: LGTM!The certificate extensions schema correctly includes:
- nsComment (same as CA)
- nsCertType with end-entity values (
client,server,objsign)- extendedKeyUsage with standard usage values
109-116: LGTM!The
get_schema_item_optionshelper correctly extracts extension options from the schema'soneOfbranches, enabling the widget to enumerate available extension types.
119-144: LGTM!The schema validation and retrieval functions properly:
- Validate schemas using jsonschema's built-in
check_schema- Raise
ImproperlyConfiguredwith clear error messages for invalid schemas- Deep-copy schemas to prevent mutation of defaults
- Support Django settings overrides while providing sensible defaults
django_x509/tests/test_extensions.py (7)
1-16: LGTM!Appropriate imports for the test module, including cryptography x509 for OID verification, Django testing utilities, and the schema constants being tested.
19-53: LGTM!Excellent test coverage for core functionality:
test_default_ca_schema_rejects_cert_only_extension: Verifies CA schema doesn't accept extendedKeyUsagetest_default_cert_schema_accepts_list_values: Verifies list-based values work and produce correct DER encoding
55-80: LGTM!Good tests for validation and normalization:
test_invalid_extension_payloads_raise_field_errors: Verifies unsupported values are rejectedtest_legacy_string_values_are_normalized: Verifies backward compatibility with comma-separated strings
82-103: LGTM!Critical edge case tests:
test_missing_critical_defaults_to_false: Verifies backward compatibility without explicit critical flagtest_nscomment_rejects_values_over_255_utf8_bytes: Verifies byte-length validation with multi-byte characterstest_nscomment_uses_long_form_der_length_for_128_bytes: Verifies DER encoding correctness at the boundary
105-171: LGTM!Comprehensive schema override tests:
test_ca_schema_setting_override: Verifies settings-based schema customization workstest_schema_override_still_rejects_unsupported_extension_names: Ensures model validation still enforces supported extensions even with custom schemastest_schema_override_still_rejects_incompatible_nscomment_shape: Ensures type validation is enforced regardless of schema
173-210: LGTM!Good unit tests for internal helper methods:
test_invalid_schema_override_raises_improperly_configured: Verifies invalid schema handlingtest_normalize_extensions_*: Tests normalization edge casestest_get_extension_values_returns_empty_list_for_unexpected_types: Tests defensive behaviortest_raise_extensions_validation_error_without_path: Tests error formattingtest_validate_supported_extensions_rejects_non_boolean_critical: Tests type validation
212-243: LGTM!Complete coverage for empty value validation:
- Tests that empty nsComment, extendedKeyUsage, and nsCertType values are properly rejected with descriptive error messages.
czarflix
force-pushed
the
issue-98-extensions-widget
branch
from
b678253 to
2148b2e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
django_x509/tests/test_admin.py (1)
95-110:⚠️ Potential issue | 🟡 Minor
cert_readonlylist contains duplicated entries.The list has
"created"and"modified"repeated 6 times each, which appears unintentional. This could cause issues if the list is used for set operations or if the length matters.🐛 Proposed fix
cert_readonly = [ "revoked", "revoked_at", "created", "modified", - "created", - "modified", - "created", - "modified", - "created", - "modified", - "created", - "modified", - "created", - "modified", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@django_x509/tests/test_admin.py` around lines 95 - 110, The cert_readonly list in django_x509/tests/test_admin.py contains duplicated "created" and "modified" entries; replace it with a deduplicated list (e.g., ["revoked", "revoked_at", "created", "modified"]) in the variable cert_readonly and update any tests that assumed the previous length to use the deduplicated list so assertions remain correct.
♻️ Duplicate comments (1)
django_x509/tests/test_admin.py (1)
178-218: 🧹 Nitpick | 🔵 TrivialConsider using
unittest.mock.patch.objectinstead of manual backup/restore.The
readonly_fieldsbackup/restore pattern intry/finallyblocks modifies class-level state and could cause test pollution. This pattern is used in bothtest_cert_extensions_widgetandtest_cert_extensions_widget_uses_overridden_schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@django_x509/tests/test_admin.py` around lines 178 - 218, Replace manual backup/restore of class-level readonly_fields in test_cert_extensions_widget and test_cert_extensions_widget_uses_overridden_schema with unittest.mock.patch.object to avoid test pollution: use patch.object(self.cert_admin, "readonly_fields", new=self.cert_admin.readonly_fields[:]) as a context manager (or decorator) around the body that constructs ma = self.cert_admin(...) and accesses ma.get_form(...).base_fields["extensions"].widget so the original cert_admin.readonly_fields is automatically restored after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@django_x509/tests/test_admin.py`:
- Around line 95-110: The cert_readonly list in django_x509/tests/test_admin.py
contains duplicated "created" and "modified" entries; replace it with a
deduplicated list (e.g., ["revoked", "revoked_at", "created", "modified"]) in
the variable cert_readonly and update any tests that assumed the previous length
to use the deduplicated list so assertions remain correct.
---
Duplicate comments:
In `@django_x509/tests/test_admin.py`:
- Around line 178-218: Replace manual backup/restore of class-level
readonly_fields in test_cert_extensions_widget and
test_cert_extensions_widget_uses_overridden_schema with
unittest.mock.patch.object to avoid test pollution: use
patch.object(self.cert_admin, "readonly_fields",
new=self.cert_admin.readonly_fields[:]) as a context manager (or decorator)
around the body that constructs ma = self.cert_admin(...) and accesses
ma.get_form(...).base_fields["extensions"].widget so the original
cert_admin.readonly_fields is automatically restored after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b02db8c-f1ec-4dc2-8180-eeff6caee788
📒 Files selected for processing (4)
django_x509/base/models.pydjango_x509/schemas.pydjango_x509/tests/test_admin.pydjango_x509/tests/test_extensions.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_admin.pydjango_x509/base/models.pydjango_x509/tests/test_extensions.pydjango_x509/schemas.py
🔇 Additional comments (22)
django_x509/tests/test_admin.py (2)
162-176: LGTM!Good test coverage for the CA extensions widget. The test verifies:
- JS/CSS assets are properly included
- Widget class is rendered
- Schema data attribute is present and correctly titled
- Extension filtering (nsComment present, extendedKeyUsage absent for CA)
257-268: LGTM!Good additions for testing edge cases:
test_revoke_action_multipleproperly tests plural message handlingtest_base_admin_default_extensions_schemaverifies BaseAdmin returns empty schema by defaultdjango_x509/schemas.py (5)
1-6: LGTM!Clean imports with appropriate dependencies for JSON schema handling and Django configuration.
8-31: LGTM!Well-designed helper functions that promote DRY principles:
_build_extension_schemacreates consistent extension object schemas_build_multi_value_schemahandles array-type value schemas with proper constraints
45-50:maxLength: 255validates characters, but model enforces byte limit.The nsComment schema specifies
maxLength: 255which counts characters, but the model validation (_validate_supported_extensions) enforces a 255-byte UTF-8 limit. Multi-byte characters could pass schema validation but fail model validation.Also applies to: 77-82
34-106: Schema definitions are well-structured and align with model validation.The CA schema correctly restricts nsCertType to CA-oriented values (
sslca,emailca,objca), while the certificate schema allows end-entity types (client,server,objsign) plusextendedKeyUsage. The PascalCase enum values forextendedKeyUsagewill be normalized to lowercase by the model before validation againstSUPPORTED_EXTENDED_KEY_USAGE_VALUES.
109-144: LGTM!Clean implementation of schema retrieval with:
get_schema_item_optionsfor extracting extension branches by name_validate_schemaproviding proper error handling withImproperlyConfigured_get_extensions_schemausingdeepcopyto prevent mutation of default schemas- Public accessors delegating to the internal helper with appropriate defaults
django_x509/base/models.py (10)
4-4: LGTM!Appropriate imports added for JSON schema validation and schema option extraction.
Also applies to: 20-20
42-57: LGTM!Well-defined constants for supported extension values. These sets are used consistently throughout the validation and certificate generation code.
179-188: LGTM!The
clean_fieldsmethod now properly:
- Passes the
excludeparameter tosuper().clean_fields()- Conditionally validates extensions only when not excluded
This follows Django's field validation conventions.
528-557: LGTM!The
_get_extensions_schemaand_normalize_extensionsmethods are well-implemented:
- Schema selection based on certificate type (CA vs end-entity)
- Proper handling of None values
- Legacy comma-separated string normalization to arrays
- Non-dict entries preserved for later validation to catch
559-595: LGTM!Error handling utilities are well-designed:
_get_best_extensions_errorrecursively finds the most specific error_format_extensions_errorproduces user-friendly path notation_raise_extensions_validation_errorprovides consistent error structure
604-607: DER length encoding only supports values up to 255 bytes.The
_get_der_length_byteshelper handles short form (< 128) and one-byte long form (128-255). For lengths ≥ 256,bytes([length])would raiseValueError. While the nsComment validation caps values at 255 bytes, consider adding a defensive assertion if this helper could be reused.
609-692: LGTM!The
_validate_supported_extensionsmethod provides thorough validation:
- Type checking for critical flag (boolean)
- Extension-specific validation for nsComment, extendedKeyUsage, and nsCertType
- Proper byte-length validation for nsComment (addresses previous review feedback)
- Consistent error paths for debugging
- Rejects unsupported extension names at the end
683-692: LGTM!The
_validate_extensionsmethod orchestrates the validation pipeline correctly:
- Gets the appropriate schema based on certificate type
- Normalizes extensions (legacy format handling)
- Validates against JSON schema
- Performs additional supported-extension validation
769-771: LGTM!Good refactoring to use
_get_extension_valueshelper for parsing extension values in_add_extensions, avoiding duplication with the validation code.Also applies to: 813-815
787-794: LGTM!The nsComment DER encoding now correctly:
- Encodes value to UTF-8 bytes
- Uses
_get_der_length_bytesfor proper length encoding- Constructs valid DER IA5String (0x16 tag)
This addresses previous review feedback about byte-length handling.
django_x509/tests/test_extensions.py (5)
1-20: LGTM!Clean imports and test class setup. The hardcoded
ns_comment_oidmatches the OID used inmodels.pyfor nsComment extensions.
22-66: LGTM!Good coverage of schema validation behavior:
- CA schema correctly rejects certificate-only extensions (
extendedKeyUsage)- Certificate schema accepts properly formatted list values
- Invalid payloads are properly rejected with field-level errors
68-103: LGTM!Excellent backward compatibility and encoding tests:
- Legacy comma-separated strings are normalized to arrays
- Missing
criticalflag defaults tofalse- UTF-8 byte limit properly enforced (128 × 2-byte chars = 256 bytes > 255 limit)
- DER long-form encoding verified for 128-byte values (
0x81 0x80)
105-176: LGTM!Comprehensive schema override testing:
- Settings-based schema overrides work correctly
- Model-level validation still enforces supported extension names even with custom schemas
- Type mismatches between schema and model validation are properly caught
- Invalid schema configurations raise
ImproperlyConfigured
178-243: LGTM!Good unit test coverage for internal methods:
_normalize_extensionsedge cases (None, non-dict entries)_get_extension_valueshandles unexpected types_raise_extensions_validation_errorformatting without path_validate_supported_extensionsrejects invalid critical flags, empty values for all extension types
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@czarflix screenshots and screen recordings added to the PR description would be very appreciated.
I added screenshots to the PR description showing the new extensions editor on the CA and certificate admin add forms, including the raw JSON fallback. Thanks for your time |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/static/django-x509/css/extensions-widget.css`:
- Around line 21-23: The CSS currently sets both column-gap: 1rem and gap:
0.75rem which causes the shorthand gap to override the column-gap; update the
declarations by removing the gap: 0.75rem line and replacing it with row-gap:
0.75rem so column-gap: 1rem is preserved and only the row spacing is set (update
the block containing column-gap and gap in
django-x509/static/django-x509/css/extensions-widget.css).
In `@django_x509/static/django-x509/js/extensions-widget.js`:
- Around line 172-186: The labels "Extension", "Critical", and fallback "Value"
are hardcoded in the JS (see selectLabel, criticalLabel, valueLabel) and should
be pulled from the widget's dataset like the existing add/remove labels; change
the code to read widget.dataset.extensionLabel, widget.dataset.criticalLabel,
and widget.dataset.valueLabel (with existing fallback behavior if missing)
instead of literal strings, and update the widget template to emit
data-extension-label, data-critical-label, and data-value-label attributes with
the translated text.
- Around line 20-24: The code assumes schema.items.oneOf is an array and calls
branches.forEach, which throws if oneOf is a non-array truthy value; change how
branches is derived and/or guarded: set branches to Array.isArray(((schema ||
{}).items || {}).oneOf) ? ((schema || {}).items || {}).oneOf : [] (or check
Array.isArray(branches) before calling branches.forEach) so non-array values
fall back to the existing unsupported behavior; update references to branches
and the forEach call accordingly to avoid the TypeError.
🪄 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: 6cfbf4c0-b5cb-4faa-95ee-de51ac2f7ba9
📒 Files selected for processing (2)
django_x509/static/django-x509/css/extensions-widget.cssdjango_x509/static/django-x509/js/extensions-widget.js
📜 Review details
🧰 Additional context used
🪛 Biome (2.4.6)
django_x509/static/django-x509/css/extensions-widget.css
[error] 23-23: Unexpected shorthand property gap after column-gap
(lint/suspicious/noShorthandPropertyOverrides)
🪛 Stylelint (17.4.0)
django_x509/static/django-x509/css/extensions-widget.css
[error] 23-23: Unexpected shorthand "gap" after "column-gap" (declaration-block-no-shorthand-property-overrides)
(declaration-block-no-shorthand-property-overrides)
|
Hi @nemesifier . I have attached the screenshots, please review when you have the time |
3 participants
Checklist
Reference to Existing Issue
Closes #98.
Description of Changes
This change replaces the raw
extensionsadmin input with a simplified schema-driven editor for CA and certificate objects.It adds separate default JSON schemas for CA and certificate extensions, allows overriding them via Django settings, validates extension payloads during field validation, and keeps a raw JSON fallback for unsupported override shapes.
The implementation stays intentionally lighter than the older
openwisp-controllercredentials widget while preserving the same schema-driven editing pattern.It also preserves backward compatibility for existing multi-value payloads by normalizing legacy comma-separated values before validation and certificate generation.
Local verification:
./run-qa-checkspython runtests.py django_x509.tests.test_extensions django_x509.tests.test_adminpython runtests.py --parallelSAMPLE_APP=1 python runtests.py --parallelScreenshot