feat: security exceptions #1486
Conversation
WalkthroughThe changes introduce a new SecurityException feature across both backend and frontend. In the backend, a new SecurityException model and related migrations, serializers, viewsets, URL routes, and permission entries have been added. Several existing models now reference security exceptions via many-to-many relationships. In the frontend, new components and forms support security exception creation and display, with updates to schema, localization messages, navigation, and data fetching logic. These updates integrate security exceptions into various parts of the application’s data model and user interface. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Model
Client->>API: GET /security-exceptions/severity
API->>Model: Retrieve severity choices
Model-->>API: Return severity options
API-->>Client: Response with severity choices
sequenceDiagram
participant User
participant Frontend
participant ModalStore
participant API
participant Database
User->>Frontend: Click “Create Security Exception”
Frontend->>ModalStore: Trigger modalSecurityExceptionCreateForm
ModalStore->>API: POST /security-exceptions with form data
API->>Database: Save security exception
Database-->>API: Confirm creation
API-->>ModalStore: Return success response
ModalStore-->>Frontend: Update UI with new security exception
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.server.ts (1)
35-37: Improve error handling for failed requests.Currently, failures are only logged to console. Consider adding proper error handling to inform users.
} else { - console.error(`Failed to fetch data for ${key}: ${response.statusText}`); + const error = await response.json().catch(() => ({ detail: response.statusText })); + throw new Error(`Failed to fetch ${key}: ${error.detail}`); }
🧹 Nitpick comments (12)
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte (2)
28-34: Add validation for ref_id field.Consider adding pattern validation to ensure consistent ref_id format across security exceptions.
<TextField {form} field="ref_id" label={m.refId()} + pattern="[A-Z]{2,}-\d{3,}" + title="Format: XX-123 (e.g., SE-001)" cacheLock={cacheLocks['ref_id']} bind:cachedValue={formDataCache['ref_id']} />
44-51: Add help text for severity levels.Users would benefit from understanding the criteria for each severity level.
<Select {form} options={model.selectOptions['severity']} field="severity" label={m.severity()} + helpText={m.severityHelpText()} cacheLock={cacheLocks['severity']} bind:cachedValue={formDataCache['severity']} />backend/core/migrations/0052_securityexception_appliedcontrol_security_exceptions_and_more.py (1)
40-64: Add database indexes for many-to-many relationships.Consider adding indexes to improve query performance when filtering by security exceptions.
Add
db_index=Trueto the foreign key fields in the through tables. This can be done in a separate migration:# Example for one of the relationships migrations.AddIndex( model_name='appliedcontrol_security_exceptions', index=models.Index(fields=['securityexception_id'], name='idx_ac_se_exception') )backend/core/urls.py (1)
74-78: Consider using explicit import for SecurityExceptionViewSet.While the route registration is correct, the
SecurityExceptionViewSetis imported via star import (from .views import *). This could lead to maintainability issues.Consider updating the imports at the top of the file:
-from .views import * +from .views import ( + SecurityExceptionViewSet, + FolderViewSet, + # ... other viewsets +)🧰 Tools
🪛 Ruff (0.8.2)
76-76:
SecurityExceptionViewSetmay be undefined, or defined from star imports(F405)
frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte (1)
153-156: Consider enabling filters for security exceptions table.While the implementation follows the pattern used in other sections, hiding filters might limit users' ability to find relevant security exceptions, especially as the list grows.
Consider these improvements:
- Enable filters to help users find relevant exceptions
- Add loading state handling for better UX
<div class="card px-4 py-2 bg-white shadow-lg max-w-full max-h-96 overflow-y-auto"> <h4 class="h4 font-semibold">{m.securityExceptions()}</h4> - <ModelTable source={data.tables['security_exceptions']} hideFilters={true} URLModel="security-exceptions" /> + <ModelTable + source={data.tables['security_exceptions']} + hideFilters={false} + URLModel="security-exceptions" + loading={data.loading} + /> </div>frontend/src/lib/components/Forms/ModelForm.svelte (1)
135-136: Address technical debt in form handling.The TODO comment and growing switch statement for form types indicate increasing technical debt. Adding new form types like SecurityExceptionForm makes this pattern less maintainable.
Consider refactoring to use a more maintainable pattern:
- Create a form registry/factory:
const formComponents = { 'perimeters': PerimeterForm, 'security-exceptions': SecurityExceptionForm, // ... other mappings };
- Replace switch with dynamic component:
<svelte:component this={formComponents[URLModel]} {form} {model} {cacheLocks} {formDataCache} {initialData} {context} />Also applies to: 283-285
backend/core/serializers.py (3)
1092-1100: Consider adding validation for requirement_assessments.While the basic setup is correct, consider adding validation to ensure that the requirement assessments belong to the same folder or have appropriate relationships.
Example validation method:
class SecurityExceptionWriteSerializer(BaseModelSerializer): requirement_assessments = serializers.PrimaryKeyRelatedField( many=True, queryset=RequirementAssessment.objects.all(), required=False ) + + def validate_requirement_assessments(self, value): + if value: + folder = self.initial_data.get('folder') + if folder and not all(ra.folder_id == folder for ra in value): + raise serializers.ValidationError( + "All requirement assessments must belong to the same folder" + ) + return value class Meta: model = SecurityException fields = "__all__"🧰 Tools
🪛 Ruff (0.8.2)
1094-1094:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
1098-1098:
SecurityExceptionmay be undefined, or defined from star imports(F405)
1102-1109: Consider adding more display fields for frontend usability.The current fields are good, but consider adding:
- status display field (similar to severity)
- formatted expiration_date field
- created_at and updated_at fields for tracking
Example additions:
class SecurityExceptionReadSerializer(BaseModelSerializer): folder = FieldsRelatedField() owners = FieldsRelatedField(many=True) severity = serializers.CharField(source="get_severity_display") + status = serializers.CharField(source="get_status_display") + expiration_date = serializers.DateTimeField(format="%Y-%m-%d") + created_at = serializers.DateTimeField(read_only=True) + updated_at = serializers.DateTimeField(read_only=True) class Meta: model = SecurityException fields = "__all__"🧰 Tools
🪛 Ruff (0.8.2)
1108-1108:
SecurityExceptionmay be undefined, or defined from star imports(F405)
9-9: Consider explicit imports instead of star imports.Replace
from core.models import *with explicit imports to improve code maintainability and avoid potential naming conflicts.Example:
-from core.models import * +from core.models import ( + SecurityException, + RequirementAssessment, + ReferentialObjectMixin, + # ... other models +)🧰 Tools
🪛 Ruff (0.8.2)
9-9:
from core.models import *used; unable to detect undefined names(F403)
backend/core/models.py (2)
1412-1414: Add database indexes for frequently queried fields.Consider adding database indexes to ref_id and status fields as they are likely to be frequently used in queries.
ref_id = models.CharField( max_length=100, null=True, blank=True, verbose_name=_("Reference ID"), + db_index=True ) status = models.CharField( verbose_name="Status", choices=Status.choices, + db_index=True, null=False, default=Status.DRAFT, max_length=10, )Also applies to: 1418-1420
1566-1571: Add docstrings to explain security_exceptions relationships.The security_exceptions field is added to multiple models without documentation explaining the purpose and usage of these relationships. Consider adding docstrings to clarify the business logic.
class Asset(NameDescriptionMixin, FolderMixin, PublishInRootFolderMixin, FilteringLabelMixin): + """ + Asset model representing ... + + Relationships: + security_exceptions (M2M): Links to security exceptions that are applicable to this asset, + allowing tracking of approved deviations from security controls. + """ # ... existing code ... class AppliedControl(NameDescriptionMixin, FolderMixin, PublishInRootFolderMixin): + """ + AppliedControl model representing ... + + Relationships: + security_exceptions (M2M): Links to security exceptions that modify or exempt this control, + documenting approved deviations from standard control implementation. + """ # ... existing code ... class Vulnerability(NameDescriptionMixin, FolderMixin, PublishInRootFolderMixin, FilteringLabelMixin): + """ + Vulnerability model representing ... + + Relationships: + security_exceptions (M2M): Links to security exceptions that are related to this vulnerability, + tracking approved acceptance or temporary mitigations. + """ # ... existing code ... class RiskScenario(NameDescriptionMixin): + """ + RiskScenario model representing ... + + Relationships: + security_exceptions (M2M): Links to security exceptions that impact this risk scenario, + documenting approved risk acceptance or alternative treatments. + """ # ... existing code ... class RequirementAssessment(AbstractBaseModel, FolderMixin, ETADueDateMixin): + """ + RequirementAssessment model representing ... + + Relationships: + security_exceptions (M2M): Links to security exceptions that affect compliance with this requirement, + tracking approved deviations or alternative implementations. + """ # ... existing code ...Also applies to: 1917-1922, 2068-2073, 2658-2663, 3440-3445
backend/core/views.py (1)
94-94: Consider replacing wildcard import with explicit imports.The
SecurityExceptionmodel is imported via wildcard import (from .models import *). While this works, explicit imports make dependencies clearer and help prevent name conflicts.Replace the wildcard import with explicit imports:
-from .models import * +from .models import ( + SecurityException, + # ... other models ... +)🧰 Tools
🪛 Ruff (0.8.2)
94-94:
from .models import *used; unable to detect undefined names(F403)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
backend/core/migrations/0052_securityexception_appliedcontrol_security_exceptions_and_more.py(1 hunks)backend/core/models.py(6 hunks)backend/core/serializers.py(6 hunks)backend/core/startup.py(5 hunks)backend/core/urls.py(1 hunks)backend/core/views.py(1 hunks)enterprise/frontend/src/lib/components/SideBar/navData.ts(1 hunks)frontend/messages/en.json(3 hunks)frontend/messages/fr.json(3 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(2 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte(1 hunks)frontend/src/lib/components/SideBar/SideBarNavigation.svelte(1 hunks)frontend/src/lib/components/SideBar/navData.ts(1 hunks)frontend/src/lib/utils/crud.ts(6 hunks)frontend/src/lib/utils/schemas.ts(7 hunks)frontend/src/lib/utils/table.ts(1 hunks)frontend/src/lib/utils/types.ts(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte(2 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts(5 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/lib/utils/types.ts
- frontend/src/lib/components/SideBar/SideBarNavigation.svelte
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/urls.py
76-76: SecurityExceptionViewSet may be undefined, or defined from star imports
(F405)
backend/core/views.py
4161-4161: SecurityException may be undefined, or defined from star imports
(F405)
4166-4166: SecurityException may be undefined, or defined from star imports
(F405)
4170-4170: SecurityException may be undefined, or defined from star imports
(F405)
backend/core/serializers.py
1094-1094: RequirementAssessment may be undefined, or defined from star imports
(F405)
1098-1098: SecurityException may be undefined, or defined from star imports
(F405)
1108-1108: SecurityException may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (25)
frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte (1)
60-68: LGTM!The security_exceptions field implementation follows the established pattern and integrates well with the existing form structure.
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte (1)
92-100: LGTM! The security exceptions field is well integrated.The implementation follows the established patterns in the codebase and is properly placed within the "more" dropdown section.
frontend/src/lib/components/SideBar/navData.ts (1)
151-155: I'll run a shell script to check how the navigation items are defined in navData and see if there's an existing convention (i.e. usage of permissions) similar to what’s suggested. This will help verify whether a permissions check is missing for the security exceptions item.frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte (1)
111-119: LGTM! The security exceptions field is well integrated.The implementation follows the established patterns in the codebase and maintains consistency with other form fields.
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (4)
49-49: LGTM!The mapping of security exceptions IDs follows the same pattern as evidences, maintaining consistency in the codebase.
116-116: LGTM!The addition of 'security_exceptions' to the list of tables to fetch is consistent with the existing pattern for 'applied-controls' and 'evidences'.
243-244: LGTM!The addition of security exception model and form to the return data follows the same pattern as other models.
330-333: LGTM!The createSecurityException action is implemented correctly using the existing nestedWriteFormAction helper.
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (4)
102-119: LGTM!The modalSecurityExceptionCreateForm function follows the same pattern as modalEvidenceCreateForm and modalMeasureCreateForm.
207-209: LGTM!The reactive statement for new security exceptions follows the same pattern as newControl and newEvidence.
381-381: LGTM!The security exceptions tab is added correctly to the TabGroup.
472-498: LGTM!The security exceptions tab content follows the same pattern as evidences tab, with proper conditional rendering for non-third-party users.
backend/core/startup.py (1)
50-50: LGTM!The security exception permissions are correctly added to each role:
- Readers and approvers get view permission
- Analysts, domain managers, and administrators get full CRUD permissions
This follows the existing permission hierarchy pattern.
Also applies to: 87-87, 195-198, 314-317, 464-467
frontend/src/lib/utils/schemas.ts (3)
137-137: LGTM!The security_exceptions field is consistently added to all relevant schemas with the same type definition:
z.string().uuid().optional().array().optional().Also applies to: 158-158, 221-222, 242-243, 411-411
496-505: LGTM!The SecurityExceptionSchema is well-defined with all necessary fields:
- Inherits name and description from NameDescriptionMixin
- Includes folder, ref_id, owners, severity, status, expiration_date, and requirement_assessments
- Uses consistent types and defaults
540-540: LGTM!The SecurityExceptionSchema is correctly mapped to 'security-exceptions' in SCHEMA_MAP.
frontend/src/lib/utils/table.ts (1)
792-795: LGTM! The security exceptions list view configuration is well-structured.The fields chosen for both head and body arrays (ref_id, name, severity, status, expiration_date) provide a good overview of security exceptions in the list view.
frontend/src/lib/utils/crud.ts (1)
755-766: Verify the severity field type configuration.The configuration looks good overall, but please verify that the severity field's valueType should be 'number'. This might affect how the frontend handles and displays severity values.
Run this script to check the severity field implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check how severity is implemented in the backend model and used in the frontend # Check backend model definition rg -A 5 "severity.*=.*models\." backend/ # Check frontend severity handling rg -A 5 "severity.*:.*number" frontend/Length of output: 2038
Severity field type configuration confirmed as consistent.
- Backend models define the severity field using both SmallIntegerField and IntegerField, ensuring it’s a numeric type.
- Frontend schemas validate severity with z.number() and the CRUD configuration specifies valueType as "number", aligning with the backend.
backend/core/serializers.py (1)
160-160: LGTM! Security exceptions integration is consistent across serializers.The security_exceptions field has been properly added to all relevant serializers:
- VulnerabilityReadSerializer
- AssetReadSerializer
- RiskScenarioReadSerializer
- AppliedControlReadSerializer
- RequirementAssessmentReadSerializer
Also applies to: 333-333, 486-486, 547-547, 969-969
backend/core/views.py (3)
4156-4160: LGTM! Class definition follows established patterns.The
SecurityExceptionViewSetclass is well-structured and follows the same pattern as other viewsets in the file.
4161-4162: LGTM! Model and filter fields are appropriate.The model is correctly set to
SecurityExceptionand the filterset fields allow filtering by related entities (requirement assessments and risk scenarios).🧰 Tools
🪛 Ruff (0.8.2)
4161-4161:
SecurityExceptionmay be undefined, or defined from star imports(F405)
4164-4170: LGTM! Action methods follow established patterns.The
severityandstatusaction methods correctly expose the enumeration choices through the API, following the pattern used by other viewsets.🧰 Tools
🪛 Ruff (0.8.2)
4166-4166:
SecurityExceptionmay be undefined, or defined from star imports(F405)
4170-4170:
SecurityExceptionmay be undefined, or defined from star imports(F405)
frontend/messages/en.json (1)
1156-1160: Introducing New Security Exception Localization Keys
This segment adds new localization keys that support the recently introduced SecurityException model. In particular, the following keys have been added:
• "more": "More" – note that the formatting for "more" was updated; please verify that this change does not conflict with other usages.
• "securityException": "Exception"
• "securityExceptions": "Exceptions"
• "addSecurityException": "Add exception"
• "expirationDate": "Expiration date"Ensure that these keys exactly match the field names and statuses in the backend model and that all related UI components correctly reference these new strings.
frontend/messages/fr.json (2)
744-744: New ‘owners’ Key Update
The key "owners" has been introduced/updated with the French translation "Propriétaires". This aligns with the ownership management updates in the application. Please confirm it is consistently used where needed.
1156-1160: Ajout des clés de localisation pour la gestion des exceptions de sécurité
Ce segment ajoute de nouvelles entrées de traduction destinées à supporter la fonctionnalité de SecurityException. Les clés ajoutées sont :
• "more": "Plus"
• "securityException": "Exception"
• "securityExceptions": "Exceptions"
• "addSecurityException": "Ajouter une exception"
• "expirationDate": "Date d'expiration"Veuillez vérifier que ces traductions sont conformes au contexte de l’application et qu’elles sont utilisées de manière cohérente dans toutes les interfaces concernées.
backend/core/migrations/0052_securityexception_appliedcontrol_security_exceptions_and_more.py
Outdated
Show resolved
Hide resolved
frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte
Outdated
Show resolved
Hide resolved
| const securityExceptionModel = getModelInfo('security_exceptions'); | ||
| const securityExceptionCreateSchema = modelSchema('security_exceptions'); | ||
| const securityExceptionCreateForm = await superValidate( | ||
| { requirement_assessments: [params.id], folder: requirementAssessment.folder.id }, | ||
| zod(exceptionCreateSchema), | ||
| { errors: false } | ||
| ); |
There was a problem hiding this comment.
Fix undefined schema reference.
The code references exceptionCreateSchema on line 197, but this schema is not defined. It should be securityExceptionCreateSchema based on the variable name pattern.
Apply this diff to fix the schema reference:
- zod(exceptionCreateSchema),
+ zod(securityExceptionCreateSchema),📝 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.
| const securityExceptionModel = getModelInfo('security_exceptions'); | |
| const securityExceptionCreateSchema = modelSchema('security_exceptions'); | |
| const securityExceptionCreateForm = await superValidate( | |
| { requirement_assessments: [params.id], folder: requirementAssessment.folder.id }, | |
| zod(exceptionCreateSchema), | |
| { errors: false } | |
| ); | |
| const securityExceptionModel = getModelInfo('security_exceptions'); | |
| const securityExceptionCreateSchema = modelSchema('security_exceptions'); | |
| const securityExceptionCreateForm = await superValidate( | |
| { requirement_assessments: [params.id], folder: requirementAssessment.folder.id }, | |
| zod(securityExceptionCreateSchema), | |
| { errors: false } | |
| ); |
This will avoid ambiguïty
…more.py ruff prettier
eric-intuitem
force-pushed
the
CA-837-implement-security-exceptions
branch
from
1e202cc to
786182a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/core/models.py (2)
1418-1424:⚠️ Potential issueIncrease max_length for status field to accommodate all status values.
The status field's max_length of 10 is too short for some of the defined choices like "in review" (9 chars) and "deprecated" (10 chars), leaving no room for potential future additions.
status = models.CharField( verbose_name="Status", choices=Status.choices, null=False, default=Status.DRAFT, - max_length=10, + max_length=20, )
1425-1429:⚠️ Potential issueAdd validation for expiration_date.
The expiration_date field should be validated to ensure it's set to a future date.
+from django.core.exceptions import ValidationError +from django.utils import timezone + class SecurityException(NameDescriptionMixin, FolderMixin, PublishInRootFolderMixin): # ... existing code ... + + def clean(self): + super().clean() + if self.expiration_date and self.expiration_date < timezone.now().date(): + raise ValidationError({'expiration_date': 'Expiration date must be in the future'})
🧹 Nitpick comments (4)
backend/core/models.py (3)
1396-1441: Add docstring to describe the SecurityException model's purpose and usage.The model would benefit from a class-level docstring explaining its purpose, when to use it, and any important considerations.
class SecurityException(NameDescriptionMixin, FolderMixin, PublishInRootFolderMixin): + """ + Represents a security exception that can be associated with various entities in the system. + + Security exceptions are used to document and track approved deviations from security requirements, + policies, or controls. They include details such as severity, status, expiration date, and owners. + """
1430-1435: Add help_text to owners field for better clarity.The owners field would benefit from help text explaining who should be assigned as owners and their responsibilities.
owners = models.ManyToManyField( User, blank=True, verbose_name="Owner", related_name="security_exceptions", + help_text=_("Users responsible for managing and monitoring this security exception"), )
1396-1441: Consider adding an active field to track exception status.To improve exception lifecycle management, consider adding an active boolean field that can be automatically set based on expiration_date.
class SecurityException(NameDescriptionMixin, FolderMixin, PublishInRootFolderMixin): # ... existing code ... + active = models.BooleanField( + default=True, + verbose_name=_("Active"), + help_text=_("Indicates if the security exception is currently active"), + ) + + def save(self, *args, **kwargs): + if self.expiration_date and self.expiration_date < timezone.now().date(): + self.active = False + super().save(*args, **kwargs)backend/core/views.py (1)
4197-4212: Improve the SecurityExceptionViewSet implementation.The implementation could be enhanced in several ways:
- Replace star import with explicit import to improve code maintainability.
- Add common viewset configurations for better functionality.
- Enhance documentation to be more descriptive.
Apply this diff to improve the implementation:
+from core.models import SecurityException + class SecurityExceptionViewSet(BaseModelViewSet): """ - API endpoint that allows security exceptions to be viewed or edited. + API endpoint that allows security exceptions to be viewed or edited. + + This viewset provides CRUD operations for managing security exceptions, + including filtering by related requirement assessments and risk scenarios, + and retrieving severity and status choices. """ model = SecurityException filterset_fields = ["requirement_assessments", "risk_scenarios"] + search_fields = ["name", "description", "ref_id"] + ordering = ["created_at"] + ordering_fields = ["created_at", "expiration_date", "severity", "status"] @action(detail=False, name="Get severity choices") def severity(self, request):🧰 Tools
🪛 Ruff (0.8.2)
4202-4202:
SecurityExceptionmay be undefined, or defined from star imports(F405)
4207-4207:
SecurityExceptionmay be undefined, or defined from star imports(F405)
4211-4211:
SecurityExceptionmay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/core/migrations/0052_securityexception_appliedcontrol_security_exceptions_and_more.py(1 hunks)backend/core/models.py(6 hunks)backend/core/views.py(1 hunks)frontend/messages/ar.json(1 hunks)frontend/messages/cs.json(1 hunks)frontend/messages/de.json(1 hunks)frontend/messages/en.json(4 hunks)frontend/messages/es.json(1 hunks)frontend/messages/fr.json(4 hunks)frontend/messages/hi.json(1 hunks)frontend/messages/id.json(1 hunks)frontend/messages/it.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/messages/sv.json(1 hunks)frontend/messages/ur.json(1 hunks)frontend/src/routes/(app)/(internal)/analytics/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts(5 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte(4 hunks)
✅ Files skipped from review due to trivial changes (8)
- frontend/messages/ro.json
- frontend/messages/ar.json
- frontend/messages/id.json
- frontend/messages/hi.json
- frontend/messages/cs.json
- frontend/src/routes/(app)/(internal)/analytics/+page.svelte
- frontend/messages/pt.json
- frontend/messages/nl.json
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/fr.json
- frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
4202-4202: SecurityException may be undefined, or defined from star imports
(F405)
4207-4207: SecurityException may be undefined, or defined from star imports
(F405)
4211-4211: SecurityException may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
backend/core/migrations/0052_securityexception_appliedcontrol_security_exceptions_and_more.py (3)
87-93: Consider making expiration_date required.Security exceptions should have a defined expiration date to ensure they are reviewed periodically.
Apply this diff to make the expiration date required:
- "expiration_date", - models.DateField( - help_text="Specify when the security exception will no longer apply", - null=True, - verbose_name="Expiration date", - ), + "expiration_date", + models.DateField( + help_text="Specify when the security exception will no longer apply", + verbose_name="Expiration date", + ),
71-85: LGTM! Well-defined status choices.The status field has a comprehensive set of states that cover the lifecycle of a security exception, from draft to deprecated.
118-167: LGTM! Well-structured many-to-many relationships.The many-to-many relationships are correctly defined with:
- Appropriate related names for reverse lookups
- Blank=True to make the relationships optional
- Clear verbose names for better admin interface display
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (3)
49-51: LGTM! Proper mapping of security exceptions.The security exceptions are correctly mapped to their IDs, similar to how evidences are handled.
118-118: LGTM! Security exceptions added to tables array.Security exceptions are correctly included in the tables array for data fetching.
334-337: LGTM! Consistent action implementation.The
createSecurityExceptionaction follows the same pattern ascreateEvidence, using thenestedWriteFormActionhelper.frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (3)
102-119: LGTM! Consistent modal implementation.The
modalSecurityExceptionCreateFormfunction follows the same pattern as other modal creation functions.
207-209: LGTM! Proper form state handling.The reactive statement correctly updates the form store when a new security exception is created.
474-501: LGTM! Well-implemented access control.The security exceptions tab is correctly hidden from third-party users using the
!$page.data.user.is_third_partycondition.backend/core/models.py (1)
1566-1571: LGTM! The security_exceptions fields are consistently defined across models.The security_exceptions ManyToManyField is consistently implemented across Asset, AppliedControl, Vulnerability, RiskScenario, and RequirementAssessment models with appropriate related_name values.
Also applies to: 1919-1924, 2070-2075, 2660-2665, 3442-3447
frontend/messages/ur.json (1)
200-200: Terminology and Translation Consistency in Urdu File
The key has been changed from "exceptionsToReview" to "acceptancesToReview" to reflect the new security exceptions feature. However, the associated translation ("نظرثانی کے لیے مستثنیات") still uses the term for “exceptions.” Please verify whether the Urdu text should also be updated (for example, to use a term that denotes “acceptances”) to ensure consistency with the new naming convention.frontend/messages/sv.json (1)
202-202: Review Translation Consistency in Swedish File
The key "acceptancesToReview" is now used instead of "exceptionsToReview", but the Swedish translation provided ("Undantag att granska") means "exceptions to review." It is advisable to reexamine the translation so that it appropriately reflects the new terminology (e.g. using a phrase that indicates "acceptances" if that is the intended concept).frontend/messages/pl.json (1)
200-200: Polish Localization: Terminology Update ConfirmedThe key has been correctly updated from "exceptionsToReview" to "acceptancesToReview" with the value "Akceptacje do przeglądu". This update is consistent with the intended terminology change across all localization files.
frontend/messages/es.json (1)
200-200: Spanish Localization: Terminology Update VerifiedThe key "acceptancesToReview" is paired with the value "Aceptaciones a revisar," which aligns perfectly with the updated terminology across the project.
frontend/messages/de.json (1)
200-200: Key Rename for Acceptance Terminology
The key previously labeled as "exceptionsToReview" has been updated to "acceptancesToReview" with its value remaining "Zu überprüfende Akzeptanzen". This change ensures that the language aligns with the broader shift in terminology across the application.frontend/messages/en.json (3)
487-488: Addition of Approval Status Keys
New localization entries "approved": "Approved" and "resolved": "Resolved" have been added to support the new security exception workflow. These keys will help indicate the status of related items in the UI.
749-749: New Owner Key Added
A new key, "owners": "Owner", has been introduced to standardize how ownership is displayed in the UI. Ensure that this key is referenced consistently where owner information is required.
1166-1170: Introduction of Security Exception Localization Keys
The following new keys have been added:
• "more": "More" – This may update formatting or context usage.
• "securityException": "Exception"
• "securityExceptions": "Exceptions"
• "addSecurityException": "Add exception"
• "expirationDate": "Expiration date"
These entries align the localization file with the new SecurityException feature by providing the necessary messaging for exception management in the frontend. Verify that these keys are used in the corresponding UI components.
| "severity", | ||
| models.SmallIntegerField( | ||
| choices=[ | ||
| (-1, "undefined"), | ||
| (0, "low"), | ||
| (1, "medium"), | ||
| (2, "high"), | ||
| (3, "critical"), | ||
| ], | ||
| default=-1, | ||
| verbose_name="Severity", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding severity validation.
The severity field allows an 'undefined' value which might lead to ambiguity in security exception tracking.
Apply this diff to make severity a required field with a more appropriate default:
- default=-1,
+ default=0,📝 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.
| "severity", | |
| models.SmallIntegerField( | |
| choices=[ | |
| (-1, "undefined"), | |
| (0, "low"), | |
| (1, "medium"), | |
| (2, "high"), | |
| (3, "critical"), | |
| ], | |
| default=-1, | |
| verbose_name="Severity", | |
| ), | |
| ), | |
| "severity", | |
| models.SmallIntegerField( | |
| choices=[ | |
| (-1, "undefined"), | |
| (0, "low"), | |
| (1, "medium"), | |
| (2, "high"), | |
| (3, "critical"), | |
| ], | |
| default=0, | |
| verbose_name="Severity", | |
| ), | |
| ), |
| @@ -197,7 +197,7 @@ | |||
| "watchlist": "Lista di controllo", | |||
| "watchlistDescription": "Elementi scaduti o in scadenza entro i prossimi 30 giorni", | |||
| "measuresToReview": "Controlli applicati da revisionare", | |||
| "exceptionsToReview": "Eccezioni da rivisionare", | |||
| "acceptancesToReview": "Eccezioni da rivisionare", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Italian Localization: Inconsistent Translation
The key "acceptancesToReview" has been updated correctly; however, its corresponding value "Eccezioni da rivisionare" still uses the term "eccezioni" (exceptions) instead of the new term "acceptances." For consistency with other language files (e.g., Spanish "Aceptaciones a revisar"), please update the Italian translation to use "Accettazioni da rivisionare." Below is a suggested diff:
-"acceptancesToReview": "Eccezioni da rivisionare",
+"acceptancesToReview": "Accettazioni da rivisionare",📝 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.
| "acceptancesToReview": "Eccezioni da rivisionare", | |
| "acceptancesToReview": "Accettazioni da rivisionare", |
Using zod to have both default and required does not work (which is consistent), so this is a simple solution.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/lib/components/Forms/Select.svelte (1)
76-76: Optimize the condition for better performance and readability.The current implementation has several areas for improvement:
- Creating a new Set on every render is inefficient
- The condition is complex and could be more maintainable
- The type safety for option labels could be improved
Consider this refactoring:
- {#if !disableDoubleDash && !$constraints?.required && !options.find( (o) => new Set( ['--', 'undefined'] ).has(o.label.toLowerCase()) )} + {#if shouldShowDefaultOption($constraints?.required, options, disableDoubleDash)}Add this helper function in the script section:
const RESERVED_LABELS = new Set(['--', 'undefined']); function shouldShowDefaultOption( required: boolean | undefined, opts: Option[], disableDoubleDash: boolean ): boolean { if (disableDoubleDash || required) { return false; } return !opts.some((o) => o.label != null && RESERVED_LABELS.has(String(o.label).toLowerCase()) ); }This refactoring:
- Improves performance by moving Set creation outside the render cycle
- Enhances readability by extracting the logic into a well-named function
- Adds proper type safety and null checks
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte (2)
11-15: Add JSDoc documentation for exported props.Consider adding JSDoc comments to document the purpose and expected values of each exported prop. This will improve maintainability and help other developers understand how to use this component.
+/** Form validation object from sveltekit-superforms */ export let form: SuperValidated<any>; +/** Model information containing foreign keys and select options */ export let model: ModelInfo; +/** Cache locks for form fields to prevent concurrent modifications */ export let cacheLocks: Record<string, CacheLock> = {}; +/** Cached form data values */ export let formDataCache: Record<string, any> = {}; +/** Initial data values for form fields */ export let initialData: Record<string, any> = {};
52-60: Clarify the purpose of disableDoubleDash in status field.The
disableDoubleDash="true"prop is used without explanation. Consider adding a comment explaining its purpose or remove if not needed.<Select {form} options={model.selectOptions['status']} field="status" label={m.status()} cacheLock={cacheLocks['status']} - disableDoubleDash="true" + disableDoubleDash="true" // Prevents -- as a valid status option bind:cachedValue={formDataCache['status']} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte(1 hunks)frontend/src/lib/components/Forms/Select.svelte(2 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.server.ts
- frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
frontend/src/lib/components/Forms/Select.svelte (1)
15-15: LGTM! Well-defined property.The new property is well-named, properly typed, and maintains backward compatibility with its default value.
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte (1)
61-68: Add future date validation for expiration_date.The expiration date should be in the future to ensure valid security exceptions.
<TextField type="date" {form} field="expiration_date" label={m.expirationDate()} + min={new Date().toISOString().split('T')[0]} cacheLock={cacheLocks['expiration_date']} bind:cachedValue={formDataCache['expiration_date']} />
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte (3)
28-34: Add required field validation for ref_id.The ref_id field should be marked as required to ensure proper tracking and identification of security exceptions.
Apply this diff:
<TextField {form} field="ref_id" label={m.refId()} + required={true} cacheLock={cacheLocks['ref_id']} bind:cachedValue={formDataCache['ref_id']} />
52-59: Add required field validation for severity.The severity field should be marked as required to ensure proper risk assessment of security exceptions.
Apply this diff:
<Select {form} options={model.selectOptions['severity']} field="severity" label={m.severity()} + required={true} cacheLock={cacheLocks['severity']} bind:cachedValue={formDataCache['severity']} />
69-76: Add future date validation for expiration_date.The expiration date should be validated to ensure it's set in the future.
Apply this diff:
<TextField type="date" {form} field="expiration_date" label={m.expirationDate()} + min={new Date().toISOString().split('T')[0]} + required={true} cacheLock={cacheLocks['expiration_date']} bind:cachedValue={formDataCache['expiration_date']} />backend/core/migrations/0052_securityexception_appliedcontrol_security_exceptions_and_more.py (2)
57-69: Improve severity field default value.The severity field allows an 'undefined' value (-1) which might lead to ambiguity in security exception tracking.
Apply this diff:
"severity", models.SmallIntegerField( choices=[ - (-1, "undefined"), (0, "low"), (1, "medium"), (2, "high"), (3, "critical"), ], - default=-1, + default=0, verbose_name="Severity", ),
86-93: Make expiration_date required.Security exceptions should have a defined expiration date to ensure they are reviewed periodically.
Apply this diff:
"expiration_date", models.DateField( help_text="Specify when the security exception will no longer apply", - null=True, verbose_name="Expiration date", ),
🧹 Nitpick comments (6)
backend/core/migrations/0052_securityexception_appliedcontrol_security_exceptions_and_more.py (1)
47-55: Add unique constraint to ref_id field.The ref_id field should be unique within a folder to ensure proper tracking and avoid confusion.
Apply this diff:
"ref_id", models.CharField( blank=True, max_length=100, null=True, + unique=True, verbose_name="Reference ID", ),frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (1)
22-29: Improve error handling in fetchJson.The error handling in fetchJson could be more informative and consistent.
Apply this diff:
async function fetchJson(url: string) { const res = await fetch(url); if (!res.ok) { - console.error(`Failed to fetch data from ${url}: ${res.statusText}`); + const error = await res.text().catch(() => res.statusText); + console.error(`Failed to fetch data from ${url}: ${error}`); + throw new Error(`HTTP ${res.status}: ${error}`); - return null; } return res.json(); }frontend/src/lib/utils/schemas.ts (1)
496-506: Strengthen validation constraints in SecurityExceptionSchema.The schema could benefit from stronger validation rules for critical fields.
Apply this diff:
export const SecurityExceptionSchema = z.object({ ...NameDescriptionMixin, folder: z.string(), - ref_id: z.string().optional(), + ref_id: z.string().min(1).max(100), owners: z.array(z.string().optional()).optional(), approver: z.string().optional().nullable(), - severity: z.number().default(-1).optional(), + severity: z.number().min(0).max(3).default(0), - status: z.string().default('draft'), + status: z.string().refine( + (val) => ['draft', 'in_review', 'approved', 'resolved', 'expired', 'deprecated'].includes(val), + { message: 'Invalid status value' } + ).default('draft'), - expiration_date: z.union([z.literal('').transform(() => null), z.string().date()]).nullish(), + expiration_date: z.string().datetime(), requirement_assessments: z.string().optional().array().optional() });backend/core/serializers.py (1)
1092-1100: Add explicit imports for SecurityException and RequirementAssessment.The serializer looks good, but the static analysis tool indicates that
SecurityExceptionandRequirementAssessmentmay be undefined. Consider replacing the star import with explicit imports to improve code clarity and avoid potential issues.-from core.models import * +from core.models import ( + SecurityException, + RequirementAssessment, + # ... other models +)🧰 Tools
🪛 Ruff (0.8.2)
1094-1094:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
1098-1098:
SecurityExceptionmay be undefined, or defined from star imports(F405)
backend/core/models.py (2)
1396-1456: Add help text to severity and status fields for better clarity.The severity and status fields would benefit from help text explaining their meaning and workflow.
severity = models.SmallIntegerField( verbose_name="Severity", choices=Severity.choices, - default=Severity.UNDEFINED + default=Severity.UNDEFINED, + help_text=_("Indicates the severity level of the security exception: undefined (-1), low (0), medium (1), high (2), critical (3)") ) status = models.CharField( verbose_name="Status", choices=Status.choices, null=False, default=Status.DRAFT, max_length=20, + help_text=_("Indicates the current status in the workflow: draft → in review → approved → resolved/expired/deprecated") )
1450-1455: Enhance expiration date validation.The expiration date validation could be more robust by checking if the date is at least one day in the future.
def clean(self): super().clean() if self.expiration_date and self.expiration_date < now().date(): + # Ensure expiration date is at least one day in the future + tomorrow = now().date() + timedelta(days=1) + if self.expiration_date < tomorrow: raise ValidationError( - {"expiration_date": "Expiration date must be in the future"} + {"expiration_date": "Expiration date must be at least one day in the future"} )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/core/migrations/0052_securityexception_appliedcontrol_security_exceptions_and_more.py(1 hunks)backend/core/models.py(6 hunks)backend/core/serializers.py(6 hunks)frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte(1 hunks)frontend/src/lib/utils/crud.ts(6 hunks)frontend/src/lib/utils/schemas.ts(7 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/serializers.py
1094-1094: RequirementAssessment may be undefined, or defined from star imports
(F405)
1098-1098: SecurityException may be undefined, or defined from star imports
(F405)
1109-1109: SecurityException may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte (1)
76-77: Add description field for security exception justification.A description field is essential for documenting the reason for the security exception and any compensating controls.
Add a description field after the expiration date:
bind:cachedValue={formDataCache['expiration_date']} /> +<TextField + type="textarea" + {form} + field="description" + label={m.description()} + required={true} + cacheLock={cacheLocks['description']} + bind:cachedValue={formDataCache['description']} +/>frontend/src/lib/utils/crud.ts (5)
238-239: LGTM! Good addition of owner and security exceptions fields.The changes enhance risk scenarios by adding ownership tracking and security exception support.
268-269: LGTM! Good addition of progress tracking and security exceptions support.The changes enhance applied controls by adding:
- Progress tracking field for better monitoring
- Owner field for accountability
- Security exceptions support
Also applies to: 275-276
345-346: LGTM! Good addition of filtering labels and security exceptions support.The changes enhance vulnerabilities by adding:
- Filtering labels for better organization
- Security exceptions support
398-399: LGTM! Good addition of EBIOS RM studies and security exceptions support.The changes enhance assets by adding:
- EBIOS RM studies integration
- Security exceptions support
755-767: LGTM! Well-structured security exceptions configuration.The security exceptions entity is properly configured with:
- Appropriate naming conventions
- Required relationships (owners, approver, folder)
- Essential fields (severity, status)
backend/core/serializers.py (6)
160-160: LGTM! Good addition of security exceptions field.The security exceptions field is properly configured as a many-to-many relationship.
333-333: LGTM! Good addition of security exceptions field.The security exceptions field is properly configured as a many-to-many relationship.
486-486: LGTM! Good addition of security exceptions field.The security exceptions field is properly configured as a many-to-many relationship.
547-547: LGTM! Good addition of security exceptions field.The security exceptions field is properly configured as a many-to-many relationship.
969-969: LGTM! Good addition of security exceptions field.The security exceptions field is properly configured as a many-to-many relationship.
1102-1110: LGTM! Well-structured read serializer for security exceptions.The serializer is properly configured with:
- Required relationships (folder, owners, approver)
- Human-readable severity display
🧰 Tools
🪛 Ruff (0.8.2)
1109-1109:
SecurityExceptionmay be undefined, or defined from star imports(F405)
backend/core/models.py (5)
1581-1586: LGTM!The many-to-many relationship between Asset and SecurityException is well-defined with appropriate options.
1934-1939: LGTM!The many-to-many relationship between AppliedControl and SecurityException is well-defined with appropriate options.
2085-2090: LGTM!The many-to-many relationship between Vulnerability and SecurityException is well-defined with appropriate options.
2675-2680: LGTM!The many-to-many relationship between RiskScenario and SecurityException is well-defined with appropriate options.
3457-3462: LGTM!The many-to-many relationship between RequirementAssessment and SecurityException is well-defined with appropriate options.
| object.evidences = object.evidences.map((evidence) => evidence.id); | ||
| object.security_exceptions = object.security_exceptions.map( | ||
| (security_exception) => security_exception.id | ||
| ); |
There was a problem hiding this comment.
Add null check for security_exceptions mapping.
The code assumes security_exceptions is always an array, which might not be true for existing data.
Apply this diff:
- object.security_exceptions = object.security_exceptions.map(
- (security_exception) => security_exception.id
- );
+ object.security_exceptions = object.security_exceptions?.map(
+ (security_exception) => security_exception.id
+ ) ?? [];📝 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.
| object.evidences = object.evidences.map((evidence) => evidence.id); | |
| object.security_exceptions = object.security_exceptions.map( | |
| (security_exception) => security_exception.id | |
| ); | |
| object.evidences = object.evidences.map((evidence) => evidence.id); | |
| object.security_exceptions = object.security_exceptions?.map( | |
| (security_exception) => security_exception.id | |
| ) ?? []; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte (1)
57-65: Consider adding help text for security exceptions.The security exceptions field could benefit from a help text explaining when and how to use security exceptions, similar to how other fields in the form provide guidance.
<AutocompleteSelect {form} multiple options={getOptions({ objects: model.foreignKeys['security_exceptions'] })} field="security_exceptions" cacheLock={cacheLocks['security_exceptions']} bind:cachedValue={formDataCache['security_exceptions']} label={m.securityExceptions()} + helpText={m.securityExceptionsHelpText()} />frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (2)
159-173: Improve error handling consistency in selectFields fetch.The error handling in the security exception selectFields fetch is inconsistent with the measure selectFields fetch. The measure fetch logs errors, but the security exception fetch silently fails.
if (securityExceptionModel.selectFields) { await Promise.all( securityExceptionModel.selectFields.map(async (selectField) => { const url = `${baseUrl}/security-exceptions/${selectField.field}/`; const data = await fetchJson(url); if (data) { securityExceptionSelectOptions[selectField.field] = Object.entries(data).map( ([key, value]) => ({ label: value, value: selectField.valueType === 'number' ? parseInt(key) : key }) ); + } else { + console.error(`Failed to fetch data for ${selectField.field}: ${response.statusText}`); } }) ); }
151-154: Add type safety to security exception form initialization.The form initialization could benefit from explicit type information to catch potential type-related issues early.
const securityExceptionCreateForm = await superValidate( - { requirement_assessments: [params.id], folder: requirementAssessment.folder.id }, + { requirement_assessments: [params.id], folder: requirementAssessment.folder.id } as SecurityExceptionFormData, zod(securityExceptionCreateSchema), { errors: false } );Consider adding a type definition:
interface SecurityExceptionFormData { requirement_assessments: string[]; folder: string; }frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (2)
481-508: Make security exceptions tab visibility logic more explicit.The security exceptions tab visibility logic combines two conditions (
tabSet === 2and!$page.data.user.is_third_party) which could be made more explicit for better maintainability.-{#if tabSet === 2 && !$page.data.user.is_third_party} +{#if tabSet === 2} + {#if !$page.data.user.is_third_party} <div class="h-full flex flex-col space-y-2 variant-outline-surface rounded-container-token p-4"> <!-- ... content ... --> </div> + {:else} + <div class="p-4 text-center text-gray-500"> + {m.securityExceptionsNotAvailableForThirdParty()} + </div> + {/if} {/if}
387-390: Consider reordering tabs for better UX.The security exceptions tab is placed after evidences, which might be confusing for users since it's only visible to non-third-party users. Consider placing it before evidences to maintain a consistent tab order for all users.
-<Tab bind:group={tabSet} name="evidences_tab" value={1}>{m.evidences()}</Tab> -<Tab bind:group={tabSet} name="security_exceptions_tab" value={2} - >{m.securityExceptions()}</Tab -> +{#if !$page.data.user.is_third_party} + <Tab bind:group={tabSet} name="security_exceptions_tab" value={1} + >{m.securityExceptions()}</Tab + > +{/if} +<Tab bind:group={tabSet} name="evidences_tab" value={2}>{m.evidences()}</Tab>backend/core/serializers.py (2)
1092-1111: Consider adding validation for requirement_assessments.The implementation follows good practices with separate read/write serializers. Consider adding validation for the requirement_assessments field to ensure data integrity.
class SecurityExceptionWriteSerializer(BaseModelSerializer): requirement_assessments = serializers.PrimaryKeyRelatedField( many=True, queryset=RequirementAssessment.objects.all(), required=False ) + + def validate_requirement_assessments(self, value): + if value and len(value) > 0: + # Ensure all requirement assessments belong to the same folder + folders = {assessment.folder for assessment in value} + if len(folders) > 1: + raise serializers.ValidationError( + "All requirement assessments must belong to the same folder" + ) + return value class Meta: model = SecurityException fields = "__all__"🧰 Tools
🪛 Ruff (0.8.2)
1094-1094:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
1098-1098:
SecurityExceptionmay be undefined, or defined from star imports(F405)
1109-1109:
SecurityExceptionmay be undefined, or defined from star imports(F405)
9-9: Consider using explicit imports instead of star imports.Replace the star import with explicit imports to improve code maintainability and address static analysis warnings:
-from core.models import * +from core.models import ( + SecurityException, + RequirementAssessment, + Folder, + RoleAssignment, + # Add other explicitly used models +)🧰 Tools
🪛 Ruff (0.8.2)
9-9:
from core.models import *used; unable to detect undefined names(F403)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/core/serializers.py(6 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(2 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte(1 hunks)frontend/src/lib/utils/crud.ts(6 hunks)frontend/src/lib/utils/types.ts(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte(2 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts(5 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/lib/utils/types.ts
- frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte
- frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte
- frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte
- frontend/src/lib/utils/crud.ts
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/serializers.py
1094-1094: RequirementAssessment may be undefined, or defined from star imports
(F405)
1098-1098: SecurityException may be undefined, or defined from star imports
(F405)
1109-1109: SecurityException may be undefined, or defined from star imports
(F405)
🪛 GitHub Actions: Frontend Linters
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (2)
frontend/src/lib/components/Forms/ModelForm.svelte (1)
34-34: LGTM!The security exception form integration follows the established patterns and is well-structured.
Also applies to: 332-334
backend/core/serializers.py (1)
160-160: LGTM! Consistent implementation of security exceptions relationship.The security_exceptions field is consistently implemented across all relevant serializers using FieldsRelatedField with many-to-many relationship support.
Also applies to: 333-333, 486-486, 547-547, 969-969
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (1)
481-506: Add help text for security exceptions section.Consider adding a help text section similar to the ones present for applied controls and evidences to maintain consistency and improve user experience.
Add this before the security exceptions section:
{#if tabSet === 2 && !$page.data.user.is_third_party} + <div class="flex items-center mb-2 px-2 text-xs space-x-2"> + <i class="fa-solid fa-info-circle" /> + <p>{m.requirementSecurityExceptionHelpText()}</p> + </div> <div class="h-full flex flex-col space-y-2 variant-outline-surface rounded-container-token p-4" >frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte (3)
11-15: Consider using more specific types.Replace
anywith a specific type interface for the form data structure. Also, consider making the default values optional using?operator.-export let form: SuperValidated<any>; +interface SecurityExceptionForm { + ref_id: string; + folder?: string; + owners: string[]; + approver?: string; + severity: string; + status: string; + expiration_date?: string; + requirement_assessments?: string[]; +} +export let form: SuperValidated<SecurityExceptionForm>; -export let cacheLocks: Record<string, CacheLock> = {}; -export let formDataCache: Record<string, any> = {}; -export let initialData: Record<string, any> = {}; +export let cacheLocks?: Record<string, CacheLock>; +export let formDataCache?: Record<string, any>; +export let initialData?: Partial<SecurityExceptionForm>;
18-18: Document the purpose of requirement_assessments.Add a comment explaining why this field is hidden and its relationship with security exceptions.
+<!-- Hidden input for linking security exceptions with requirement assessments --> <HiddenInput {form} field="requirement_assessments" />
21-21: Extract hardcoded values to constants.Move hardcoded endpoint filters to named constants for better maintainability.
+<script lang="ts"> + const FOLDER_TYPES = ['DO', 'GL']; + const USERS_FILTER = 'is_third_party=false'; + + // ... rest of the script +</script> - optionsEndpoint="folders?content_type=DO&content_type=GL" + optionsEndpoint={`folders?content_type=${FOLDER_TYPES.join('&content_type=')}`} - optionsEndpoint="users?is_third_party=false" + optionsEndpoint={`users?${USERS_FILTER}`} - optionsEndpoint="users?is_third_party=false" + optionsEndpoint={`users?${USERS_FILTER}`}Also applies to: 38-38, 47-47
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte(2 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte
- frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte
- frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte
- frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
🔇 Additional comments (4)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (3)
101-118: LGTM! Well-structured modal creation function.The implementation follows the established pattern for modal creation functions and correctly handles the security exception form.
214-216: LGTM! Proper form store update.The reactive statement correctly updates the form store with new security exceptions.
388-390: LGTM! Well-integrated tab.The security exceptions tab is properly integrated into the tab group with appropriate third-party access control.
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte (1)
28-34: Add required field validation and description field.Critical fields should be marked as required, and a description field should be added for documenting the exception's justification.
Also applies to: 54-70
| <TextField | ||
| type="date" | ||
| {form} | ||
| field="expiration_date" | ||
| label={m.expirationDate()} | ||
| cacheLock={cacheLocks['expiration_date']} | ||
| bind:cachedValue={formDataCache['expiration_date']} | ||
| /> |
There was a problem hiding this comment.
Add validation for expiration date.
The expiration date should be validated to ensure it's not in the past and has a reasonable future date (e.g., not more than 1 year from now).
<TextField
type="date"
{form}
field="expiration_date"
label={m.expirationDate()}
+ min={new Date().toISOString().split('T')[0]}
+ max={new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString().split('T')[0]}
cacheLock={cacheLocks['expiration_date']}
bind:cachedValue={formDataCache['expiration_date']}
/>📝 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.
| <TextField | |
| type="date" | |
| {form} | |
| field="expiration_date" | |
| label={m.expirationDate()} | |
| cacheLock={cacheLocks['expiration_date']} | |
| bind:cachedValue={formDataCache['expiration_date']} | |
| /> | |
| <TextField | |
| type="date" | |
| {form} | |
| field="expiration_date" | |
| label={m.expirationDate()} | |
| min={new Date().toISOString().split('T')[0]} | |
| max={new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString().split('T')[0]} | |
| cacheLock={cacheLocks['expiration_date']} | |
| bind:cachedValue={formDataCache['expiration_date']} | |
| /> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (1)
148-174: Add error handling for select field data fetching.While the implementation follows the existing pattern, it's missing error handling for failed select field data fetches, unlike the measure model setup which logs errors.
Add error handling similar to the measure model:
if (data) { securityExceptionSelectOptions[selectField.field] = Object.entries(data).map( ([key, value]) => ({ label: value, value: selectField.valueType === 'number' ? parseInt(key) : key }) ); + } else { + console.error(`Failed to fetch data for ${selectField.field}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts(5 hunks)
🔇 Additional comments (4)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (4)
49-50: LGTM! Robust null handling.The implementation correctly handles potential null/undefined values for security exceptions using optional chaining and nullish coalescing, following the same pattern as evidence mapping.
102-102: LGTM! Consistent table integration.The security exceptions table is integrated using the same pattern as other entities, maintaining consistency in data fetching and error handling.
188-189: LGTM! Consistent return value updates.The security exception model and form are correctly added to the return object, following the established naming convention and pattern.
275-278: LGTM! Consistent action implementation.The security exception creation action correctly uses the nestedWriteFormAction helper and follows the same pattern as evidence creation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/views.py (1)
4202-4216: LGTM! Consider using explicit imports.The
SecurityExceptionViewSetimplementation follows Django REST framework conventions with proper model configuration and choice exposure. However, consider replacing the wildcard import with an explicit import ofSecurityExceptionfrom.modelsto improve code clarity and avoid potential naming conflicts.-from .models import * +from .models import ( + SecurityException, + # ... other models ... +)🧰 Tools
🪛 Ruff (0.8.2)
4207-4207:
SecurityExceptionmay be undefined, or defined from star imports(F405)
4212-4212:
SecurityExceptionmay be undefined, or defined from star imports(F405)
4216-4216:
SecurityExceptionmay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/views.py(6 hunks)frontend/src/lib/components/DetailView/DetailView.svelte(2 hunks)frontend/src/lib/utils/crud.ts(7 hunks)frontend/src/lib/utils/load.ts(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
4207-4207: SecurityException may be undefined, or defined from star imports
(F405)
4212-4212: SecurityException may be undefined, or defined from star imports
(F405)
4216-4216: SecurityException may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: ruff (3.12)
🔇 Additional comments (6)
frontend/src/lib/utils/load.ts (1)
128-129: LGTM! Clean addition of the disableAddDeleteButtons property.The property is correctly passed through from the model's reverseForeignKeyFields to control UI behavior.
frontend/src/lib/components/DetailView/DetailView.svelte (1)
460-473: LGTM! Clean implementation of conditional button rendering.The changes correctly implement the conditional rendering of add/delete buttons based on the model's disableAddDeleteButtons property while maintaining the table display functionality.
frontend/src/lib/utils/crud.ts (3)
102-102: LGTM! Clean addition of disableAddDeleteButtons to ForeignKeyField interface.The interface update properly supports the new functionality for controlling UI button visibility.
748-771: LGTM! Well-structured security exceptions configuration.The security-exceptions entity is well-configured with:
- Proper naming and localization properties
- Required foreign key fields for owners, approver, and folder
- Select fields for severity and status
- Comprehensive reverse foreign key fields with disabled add/delete buttons
231-232: LGTM! Consistent integration with existing entities.Security exceptions are properly integrated with risk-scenarios, applied-controls, vulnerabilities, assets, and requirement-assessments through foreign key relationships.
Also applies to: 268-269, 338-339, 391-392, 494-495
backend/core/views.py (1)
402-402: LGTM! Security exceptions field consistently added across models.The
security_exceptionsfield has been properly added to the filterset fields of Asset, Vulnerability, AppliedControl, RiskScenario, and RequirementAssessment models, establishing a consistent many-to-many relationship pattern.Also applies to: 647-647, 1061-1061, 1568-1568, 3819-3819
Add security exceptions
Summary by CodeRabbit