feat: created backend checking if the objects on creation/edit are relevant in domain context #1769
Conversation
… or onetoone elements
WalkthroughThe pull request introduces a new method, Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/core/views.py (1)
backend/iam/models.py (3)
Folder(57-179)RoleAssignment(565-815)get_accessible_object_ids(675-757)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: startup-functional-test (3.12)
- 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: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/core/views.py (2)
263-263: Enforces domain context validation on create.This addition ensures that when creating new objects, their related objects are validated against the current folder's access permissions.
272-272: Enforces domain context validation on update.By passing the existing instance to
_validate_related_objects_access, this change extends validation to the update flow to ensure related objects remain accessible in the new folder context.
backend/core/views.py
Outdated
| def _validate_related_objects_access(self, request: Request, instance=None): | ||
| folder_id = request.data.get("folder") | ||
| if not folder_id: | ||
| raise ValidationError( | ||
| {"folder": "Folder ID is required in the request data"} | ||
| ) | ||
|
|
||
| current_folder = get_object_or_404(Folder, id=folder_id) | ||
|
|
||
| model_class = self.get_queryset().model | ||
| fields = (instance or model_class())._meta.get_fields() | ||
|
|
||
| for field in fields: | ||
| if not hasattr(field, "related_model") or field.related_model is None: | ||
| continue | ||
|
|
||
| field_value = request.data.get(field.name) | ||
| if not field_value or field_value == -1: | ||
| continue | ||
|
|
||
| accessible_ids = set( | ||
| RoleAssignment.get_accessible_object_ids( | ||
| current_folder, request.user, field.related_model | ||
| )[0] | ||
| ) | ||
|
|
||
| if field.many_to_many: | ||
| related_ids = ( | ||
| field_value if isinstance(field_value, list) else [field_value] | ||
| ) | ||
| if not all(uuid.UUID(str(id)) in accessible_ids for id in related_ids): | ||
| raise ValidationError( | ||
| { | ||
| field.name: "Some objects are not accessible in the current folder" | ||
| } | ||
| ) | ||
|
|
||
| elif field.one_to_one or field.many_to_one: | ||
| related_id = uuid.UUID(str(field_value)) | ||
| if related_id not in accessible_ids: | ||
| raise ValidationError( | ||
| { | ||
| field.name: "The object is not accessible in the current folder" | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
Improve error handling for invalid UUID inputs.
Currently, if field_value is not a valid UUID string, calling uuid.UUID(str(field_value)) will raise an unhandled ValueError, resulting in a 500 status error. For better user feedback and to avoid unexpected crashes, wrap UUID parsing in a try/except block and raise a ValidationError for invalid inputs.
Below is a suggested partial diff demonstrating more robust error handling:
--- a/backend/core/views.py
+++ b/backend/core/views.py
@@ -301,7 +301,15 @@ class BaseModelViewSet(viewsets.ModelViewSet):
if field.many_to_many:
related_ids = (
field_value if isinstance(field_value, list) else [field_value]
)
- if not all(uuid.UUID(str(id)) in accessible_ids for id in related_ids):
+ try:
+ parsed_ids = [uuid.UUID(str(obj_id)) for obj_id in related_ids]
+ except ValueError:
+ raise ValidationError(
+ {field.name: "Invalid UUID format in related object references"}
+ )
+ if not all(obj_id in accessible_ids for obj_id in parsed_ids):
raise ValidationError(
{
field.name: "Some objects are not accessible in the current folder"
@@ -312,7 +320,13 @@ class BaseModelViewSet(viewsets.ModelViewSet):
elif field.one_to_one or field.many_to_one:
- related_id = uuid.UUID(str(field_value))
+ try:
+ related_id = uuid.UUID(str(field_value))
+ except ValueError:
+ raise ValidationError(
+ {field.name: "Invalid UUID format for the related object reference"}
+ )
if related_id not in accessible_ids:
raise ValidationError(
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _validate_related_objects_access(self, request: Request, instance=None): | |
| folder_id = request.data.get("folder") | |
| if not folder_id: | |
| raise ValidationError( | |
| {"folder": "Folder ID is required in the request data"} | |
| ) | |
| current_folder = get_object_or_404(Folder, id=folder_id) | |
| model_class = self.get_queryset().model | |
| fields = (instance or model_class())._meta.get_fields() | |
| for field in fields: | |
| if not hasattr(field, "related_model") or field.related_model is None: | |
| continue | |
| field_value = request.data.get(field.name) | |
| if not field_value or field_value == -1: | |
| continue | |
| accessible_ids = set( | |
| RoleAssignment.get_accessible_object_ids( | |
| current_folder, request.user, field.related_model | |
| )[0] | |
| ) | |
| if field.many_to_many: | |
| related_ids = ( | |
| field_value if isinstance(field_value, list) else [field_value] | |
| ) | |
| if not all(uuid.UUID(str(id)) in accessible_ids for id in related_ids): | |
| raise ValidationError( | |
| { | |
| field.name: "Some objects are not accessible in the current folder" | |
| } | |
| ) | |
| elif field.one_to_one or field.many_to_one: | |
| related_id = uuid.UUID(str(field_value)) | |
| if related_id not in accessible_ids: | |
| raise ValidationError( | |
| { | |
| field.name: "The object is not accessible in the current folder" | |
| } | |
| ) | |
| def _validate_related_objects_access(self, request: Request, instance=None): | |
| folder_id = request.data.get("folder") | |
| if not folder_id: | |
| raise ValidationError( | |
| {"folder": "Folder ID is required in the request data"} | |
| ) | |
| current_folder = get_object_or_404(Folder, id=folder_id) | |
| model_class = self.get_queryset().model | |
| fields = (instance or model_class())._meta.get_fields() | |
| for field in fields: | |
| if not hasattr(field, "related_model") or field.related_model is None: | |
| continue | |
| field_value = request.data.get(field.name) | |
| if not field_value or field_value == -1: | |
| continue | |
| accessible_ids = set( | |
| RoleAssignment.get_accessible_object_ids( | |
| current_folder, request.user, field.related_model | |
| )[0] | |
| ) | |
| if field.many_to_many: | |
| related_ids = ( | |
| field_value if isinstance(field_value, list) else [field_value] | |
| ) | |
| try: | |
| parsed_ids = [uuid.UUID(str(obj_id)) for obj_id in related_ids] | |
| except ValueError: | |
| raise ValidationError( | |
| {field.name: "Invalid UUID format in related object references"} | |
| ) | |
| if not all(obj_id in accessible_ids for obj_id in parsed_ids): | |
| raise ValidationError( | |
| { | |
| field.name: "Some objects are not accessible in the current folder" | |
| } | |
| ) | |
| elif field.one_to_one or field.many_to_one: | |
| try: | |
| related_id = uuid.UUID(str(field_value)) | |
| except ValueError: | |
| raise ValidationError( | |
| {field.name: "Invalid UUID format for the related object reference"} | |
| ) | |
| if related_id not in accessible_ids: | |
| raise ValidationError( | |
| { | |
| field.name: "The object is not accessible in the current folder" | |
| } | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/views.py (1)
277-281: Consider combining theseifbranches
According to the static analysis hint, you can combine the consecutive checks onself.modelto simplify the logic. A possible refactor:if not folder_id: - if self.model == Folder: - return - elif self.model == User: - return - raise ValidationError({"folder": "Folder ID is required in the request data"}) + if self.model in [Folder, User]: + return + raise ValidationError( + {"folder": "Folder ID is required in the request data"} + )🧰 Tools
🪛 Ruff (0.8.2)
278-281: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/core/views.py (1)
backend/iam/models.py (4)
Folder(57-179)User(340-546)RoleAssignment(565-815)get_accessible_object_ids(675-757)
🪛 Ruff (0.8.2)
backend/core/views.py
278-281: Combine if branches using logical or operator
Combine if branches
(SIM114)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
backend/core/views.py (3)
263-263: Ensuring related objects are accessible before create
This invocation of_validate_related_objects_access(request)before thesuper().create()call properly checks if any referenced related objects are accessible within the current domain/folder context.
272-272: Ensuring related objects remain accessible before update
Similarly, calling_validate_related_objects_access(request, instance=self.get_object())beforesuper().update()helps verify that previously persisted references still conform to the updated folder-access constraints.
316-316: Potential unhandledValueErrorfor invalid UUID inputs
Iffield_valueis not a valid UUID string, callinguuid.UUID(str(field_value))raises aValueErrorthat will result in an unhandled exception. Consider wrapping this in a try/except and raising aValidationErrorfor more graceful error handling (as previously suggested).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/core/views.py (1)
275-354:⚠️ Potential issueAdd proper UUID validation to prevent server errors.
The new
_validate_related_objects_accessmethod is well-structured but lacks robust UUID validation. Currently, iffield_valueis not a valid UUID string, calls touuid.UUID(str(field_value))will raise an unhandledValueError, resulting in a 500 status error instead of a proper validation error.Apply this diff for better error handling:
if field.many_to_many: related_ids = ( field_value if isinstance(field_value, list) else [field_value] ) - if not all(uuid.UUID(str(id)) in accessible_ids for id in related_ids): + try: + parsed_ids = [uuid.UUID(str(obj_id)) for obj_id in related_ids] + except ValueError: + raise ValidationError( + {field.name: "Invalid UUID format in related object references"} + ) + if not all(obj_id in accessible_ids for obj_id in parsed_ids): raise PermissionDenied( { field.name: "Some objects are not accessible in the current folder" } ) elif field.one_to_one or field.many_to_one: - related_id = uuid.UUID(str(field_value)) + try: + related_id = uuid.UUID(str(field_value)) + except ValueError: + raise ValidationError( + {field.name: "Invalid UUID format for the related object reference"} + ) if related_id not in accessible_ids: raise PermissionDenied( { field.name: "The object is not accessible in the current folder" } )🧰 Tools
🪛 Ruff (0.8.2)
284-284:
Perimetermay be undefined, or defined from star imports(F405)
287-289: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
303-307: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (2)
backend/core/views.py (2)
287-289: Improve exception chaining for better error traceability.When raising exceptions from within an exception handler, it's best practice to use
raise ... from errto maintain the exception chain.Update the exception handling:
try: perimeter_id = uuid.UUID(str(request.data["perimeter"])) perimeter = get_object_or_404(Perimeter, id=perimeter_id) folder_id = perimeter.folder.id except (ValueError, AttributeError, KeyError, TypeError) as err: - raise ValidationError( + raise ValidationError( {"folder": "Invalid perimeter or missing folder in perimeter"} - ) + ) from err🧰 Tools
🪛 Ruff (0.8.2)
287-289: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
303-307: Improve exception chaining for better error traceability.When raising exceptions from within an exception handler, it's best practice to use
raise ... from errto maintain the exception chain.Update the exception handling:
try: risk_assessment_id = uuid.UUID(str(request.data["risk_assessment"])) risk_assessment = get_object_or_404( RiskAssessment, id=risk_assessment_id ) folder_id = risk_assessment.folder.id except (ValueError, AttributeError, KeyError, TypeError) as err: - raise ValidationError( + raise ValidationError( { "risk_assessment": "Invalid risk assessment or missing folder in risk assessment" } - ) + ) from err🧰 Tools
🪛 Ruff (0.8.2)
303-307: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/core/views.py (3)
backend/core/models.py (1)
update(678-690)backend/core/serializers.py (2)
update(59-67)update(748-762)backend/iam/models.py (3)
Folder(57-179)User(340-546)RoleAssignment(565-815)
🪛 Ruff (0.8.2)
backend/core/views.py
284-284: Perimeter may be undefined, or defined from star imports
(F405)
287-289: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
303-307: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
backend/core/views.py (6)
263-263: LGTM! The validation logic is correctly integrated into the create flow.The call to
_validate_related_objects_accessis appropriately placed after processing the request data and labels, ensuring proper validation of related objects before creation.
272-272: LGTM! The validation logic is correctly integrated into the update flow.The call to
_validate_related_objects_accessis appropriately placed with the current instance parameter, allowing the validation to consider the context of the existing object during updates.
275-278: LGTM! Good method initialization with clear parameter structure.The method signature is well-designed with an optional instance parameter that distinguishes between create and update operations.
309-315: Good exemption for Folder and User models.The exemption for Folder and User models is a sensible design choice, as these models may not always be tied to a specific folder context.
316-317: LGTM! Proper folder retrieval using get_object_or_404.Using get_object_or_404 ensures a clean 404 response when the folder doesn't exist, rather than causing a server error.
321-327: LGTM! Effective field filtering.The code efficiently skips non-relationship fields and null/empty values, focusing only on the relevant related object fields that need validation.
| ): | ||
| try: | ||
| perimeter_id = uuid.UUID(str(request.data["perimeter"])) | ||
| perimeter = get_object_or_404(Perimeter, id=perimeter_id) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure the Perimeter class is explicitly imported.
There's a potential undefined reference to Perimeter in the code.
The static analysis indicates that Perimeter may be undefined or imported via star import. Let's check if it's properly imported:
🏁 Script executed:
#!/bin/bash
# Check if Perimeter is properly imported
grep -n "import.*Perimeter" backend/core/views.py || grep -n "from.*import.*Perimeter" backend/core/views.py || grep -n "from.*import \*" backend/core/views.pyLength of output: 246
Action Required: Explicitly import the Perimeter class.
- In
backend/core/views.py, the code at line 284 usesget_object_or_404(Perimeter, id=perimeter_id). - The file currently imports modules using star imports (e.g.,
from .models import *), which meansPerimeteris only indirectly imported. - To avoid potential undefined reference issues and improve code clarity, please add an explicit import. For example:
from .models import Perimeter
🧰 Tools
🪛 Ruff (0.8.2)
284-284: Perimeter may be undefined, or defined from star imports
(F405)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/core/views.py (1)
276-375:⚠️ Potential issueHandle potential
ValueErrorwhen parsing UUIDs.Currently, calling
uuid.UUID(str(id))without atry/excepthere can raise an unhandledValueErrorfor invalid inputs, resulting in a 500 error. Wrap it in atry/exceptand raise aValidationErrorfor better user feedback:+ try: + if not all(uuid.UUID(str(obj_id)) in accessible_ids for obj_id in related_ids): + raise PermissionDenied( + {field.name: "Some objects are not accessible in the current folder"} + ) + except ValueError: + raise ValidationError( + {field.name: "Invalid UUID format in related object references"} + )🧰 Tools
🪛 Ruff (0.8.2)
285-285:
Perimetermay be undefined, or defined from star imports(F405)
288-290: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
304-308: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (2)
backend/core/views.py (2)
276-375: Remove debug print statements.Consider removing or replacing the
print(related_ids)andprint(related_id)statements (lines 354, 363) to maintain clean production logs.🧰 Tools
🪛 Ruff (0.8.2)
285-285:
Perimetermay be undefined, or defined from star imports(F405)
288-290: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
304-308: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
276-375: Useraise ... from Nonefor clarity inexceptblocks.When raising
ValidationErrorinside theexcept (ValueError, AttributeError, KeyError, TypeError):blocks at lines 288-290 and 304-308, consider appendingfrom Noneto distinguish these from secondary errors in exception handling.🧰 Tools
🪛 Ruff (0.8.2)
285-285:
Perimetermay be undefined, or defined from star imports(F405)
288-290: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
304-308: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/core/views.py (2)
backend/core/models.py (3)
RiskScenario(2579-2885)update(678-690)RiskAssessment(2240-2570)backend/iam/models.py (3)
Folder(57-179)User(340-546)RoleAssignment(565-815)
🪛 Ruff (0.8.2)
backend/core/views.py
285-285: Perimeter may be undefined, or defined from star imports
(F405)
288-290: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
304-308: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
backend/core/views.py (4)
88-88: Safe import ofRiskScenario.No issues found with this newly added import.
264-264: Validation call on create.It’s good to see
_validate_related_objects_accessinvoked here to ensure only accessible related objects can be set during the create process.
273-273: Validation call on update.Likewise, calling
_validate_related_objects_accessinside the update method enforces consistent security checks when modifying objects.
276-375: Use explicit import forPerimeter.This was previously flagged. To avoid potential undefined references and improve clarity, explicitly import
Perimeterinstead of relying on wildcard (*) imports.🧰 Tools
🪛 Ruff (0.8.2)
285-285:
Perimetermay be undefined, or defined from star imports(F405)
288-290: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
304-308: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
backend/app_tests/api/test_api_compliance_assessments.py (1)
232-281: 💡 Verification agent🧩 Analysis chain
Consider enabling the commented-out test for out-of-scope objects
This commented-out test case is essential for verifying the PR's core functionality - ensuring that only relevant objects in the domain context can be used during creation/editing.
The current test only verifies the "happy path" (objects in the same folder), but doesn't validate that attempts to use out-of-scope objects are properly rejected.
Is there a reason this test is commented out? Consider enabling it to ensure complete test coverage of the new backend functionality described in the PR objectives.
🏁 Script executed:
#!/bin/bash # Check if similar tests for out-of-scope objects are implemented in other test files echo "Checking for out-of-scope object tests in other API test files..." rg -A 3 -B 3 "test.*fails.*with.*out.*of.*scope.*object" backend/app_tests/api/Length of output: 2903
Action Required: Enable Out-of-Scope Test for Compliance Assessments
The test in
backend/app_tests/api/test_api_compliance_assessments.pyfor out-of-scope objects is currently commented out, while similar tests in other modules (risk scenarios, requirement assessments, evidences) are implemented. Enabling this test will help ensure that attempts to update a compliance assessment with out-of-scope objects are properly rejected, maintaining a consistent test coverage for all API endpoints.
- Review the commented-out test block in lines 232–281.
- If the test is failing due to known limitations, add a TODO with a clear explanation; otherwise, enable the test to validate non-happy-path scenarios.
🧹 Nitpick comments (5)
backend/app_tests/api/test_api_risk_scenarios.py (1)
415-415: Update folder creation to match pattern in other tests.If the test is uncommented, this line should be updated to clearly denote that this folder is intentionally out of scope by not setting a parent_folder relationship to test.folder.
- folder = Folder.objects.create(name="test2") + # Create an intentionally out-of-scope folder (no parent relationship) + folder = Folder.objects.create(name="test2_out_of_scope")backend/core/views.py (4)
88-88: Prefer explicit imports over star imports.There is a potential undefined reference to
RiskScenarioif it's only brought in via a star import. Explicitly importingRiskScenariocan improve code clarity and avoid overshadowing existing classes or future additions.-from .models import * +from .models import RiskAssessment, RiskScenario, ...
288-290: Raise underlying exceptions with contextual chaining.When re-raising inside an
except, consider usingraise ValidationError(...) from e(orfrom None) to preserve the original traceback context. This helps distinguish errors in exception handling from the original exception.except (ValueError, AttributeError, KeyError, TypeError) as e: - raise ValidationError({"folder": "Invalid perimeter or missing folder in perimeter"}) + raise ValidationError( + {"folder": "Invalid perimeter or missing folder in perimeter"} + ) from e🧰 Tools
🪛 Ruff (0.8.2)
288-290: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
312-315: Clarify model exemptions.This bypass logic for
Folder,User, andReferenceControlis valid if these are the only models that do not require an associated folder. If future models also require skipping folder checks, consider making this mechanism more extensible.🧰 Tools
🪛 Ruff (0.8.2)
314-314:
ReferenceControlmay be undefined, or defined from star imports(F405)
357-358: Remove or replace raw print statements.Leaving
print()calls in production code can clutter logs or break JSON responses. Use structured logging instead (e.g.,logger.debug).- print(related_ids) - print(related_id) + logger.debug("Related IDs: %s", related_ids) + logger.debug("Related ID: %s", related_id)Also applies to: 367-367
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app_tests/api/test_api_compliance_assessments.py(2 hunks)backend/app_tests/api/test_api_evidences.py(2 hunks)backend/app_tests/api/test_api_requirement_assessments.py(2 hunks)backend/app_tests/api/test_api_risk_acceptances.py(2 hunks)backend/app_tests/api/test_api_risk_assessments.py(2 hunks)backend/app_tests/api/test_api_risk_scenarios.py(2 hunks)backend/core/views.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
backend/app_tests/api/test_api_evidences.py (1)
backend/core/views.py (1)
create(258-265)
backend/core/views.py (2)
backend/core/models.py (3)
RiskScenario(2579-2885)Perimeter(1387-1430)RiskAssessment(2240-2570)backend/iam/models.py (1)
Folder(57-179)
backend/app_tests/api/test_api_requirement_assessments.py (2)
backend/iam/models.py (1)
Folder(57-179)backend/core/models.py (5)
create(2095-2097)ComplianceAssessment(2888-3580)Framework(986-1061)RequirementAssessment(3583-3754)RequirementNode(1064-1134)
🪛 Ruff (0.8.2)
backend/app_tests/api/test_api_evidences.py
163-163: Local variable folder is assigned to but never used
Remove assignment to unused variable folder
(F841)
backend/core/views.py
285-285: Perimeter may be undefined, or defined from star imports
(F405)
288-290: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
304-308: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
314-314: ReferenceControl may be undefined, or defined from star imports
(F405)
backend/app_tests/api/test_api_requirement_assessments.py
213-213: Local variable folder is assigned to but never used
Remove assignment to unused variable folder
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
backend/app_tests/api/test_api_evidences.py (1)
169-206: Good test coverage improvements.The new steps for
update_objectwith multipart data, including the attachment, look correct and thorough. This test scenario effectively validates file uploads and structure updates in one go.backend/app_tests/api/test_api_risk_acceptances.py (1)
183-230: Robust update testing approach.The extended update operation with multiple approvers and additional fields appears valid and thoroughly tested.
backend/app_tests/api/test_api_risk_assessments.py (1)
173-212: Comprehensive test coverage for multi-step updates.Importing and using two risk matrices, then verifying updates across both perimeter and matrix references, provides strong assurance of correct behavior.
backend/app_tests/api/test_api_requirement_assessments.py (1)
214-297: Enhanced update parameters increase test reliability.Including a second compliance assessment, applied controls, and updated observation helps ensure the requirement assessment logic is tested comprehensively.
backend/app_tests/api/test_api_risk_scenarios.py (1)
334-334: Appropriate change to establish folder hierarchy.Adding the parent-child relationship between folders aligns with the new domain context validation functionality. This change enables testing that objects within the folder hierarchy are properly accessible.
backend/core/views.py (4)
264-264: Validation hook placement looks appropriate.Invoking
_validate_related_objects_accessright after_process_request_dataensures that the fully processed request payload is subjected to folder/permissions checks. This ordering is sensible and avoids partial data validations.
273-273: Consistency in update method.Similarly, calling
_validate_related_objects_accessforupdatemaintains consistent and centralized permission checks across create/update flows. This placement is logical and consistent with the create method flow.
304-308: Duplicate recommendation for improved exception chaining.Same pattern as lines 288-290. Use exception chaining to preserve the context of the original error when re-raising.
🧰 Tools
🪛 Ruff (0.8.2)
304-308: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
358-359: Wrap UUID parsing in a try/except to avoid unhandled 500 errors.Calling
uuid.UUID(str(id))may raise aValueErrorif the string is invalid, causing a 500 response. For better robustness, wrap these calls and raise aValidationErrorto deliver clearer feedback to the client.- if not all(uuid.UUID(str(id)) in accessible_ids for id in related_ids): + try: + parsed_ids = [uuid.UUID(str(obj_id)) for obj_id in related_ids] + except ValueError: + raise ValidationError({"folder": "Invalid UUID in related objects"}) + if not all(parsed_id in accessible_ids for parsed_id in parsed_ids): ... similarly for the single related_id = uuid.UUID(str(field_value)) ...Also applies to: 366-367
backend/app_tests/api/test_api_compliance_assessments.py (1)
185-190: Appropriate setup for testing domain-scoped updatesThe additions to import Documents and multiple Frameworks, along with creating perimeter objects in the same folder, provide good test coverage for the folder-based access validation functionality being implemented in this PR.
| folder = Folder.objects.create(name="test2") | ||
| applied_control = AppliedControl.objects.create(name="test", folder=test.folder) | ||
| applied_control2 = AppliedControl.objects.create( | ||
| name="test2", folder=test.folder | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable to eliminate code smell.
The variable folder at line 163 is never referenced. This could introduce confusion and should be removed or utilized if intended for further testing.
- folder = Folder.objects.create(name="test2")📝 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.
| folder = Folder.objects.create(name="test2") | |
| applied_control = AppliedControl.objects.create(name="test", folder=test.folder) | |
| applied_control2 = AppliedControl.objects.create( | |
| name="test2", folder=test.folder | |
| ) | |
| applied_control = AppliedControl.objects.create(name="test", folder=test.folder) | |
| applied_control2 = AppliedControl.objects.create( | |
| name="test2", folder=test.folder | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
163-163: Local variable folder is assigned to but never used
Remove assignment to unused variable folder
(F841)
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Framework") | ||
| folder = Folder.objects.create(name="test2") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable to maintain clarity.
The folder variable at line 213 is never used, which can be misleading. Suggest removing it or putting it to actual use.
- folder = Folder.objects.create(name="test2")📝 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.
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Framework") | |
| folder = Folder.objects.create(name="test2") | |
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Framework") |
🧰 Tools
🪛 Ruff (0.8.2)
213-213: Local variable folder is assigned to but never used
Remove assignment to unused variable folder
(F841)
| """def test_update_risk_scenarios_fails_with_out_of_scope_object(self, test): | ||
|
|
||
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Risk matrix") | ||
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Risk matrix2") | ||
| folder = Folder.objects.create(name="test2") | ||
| risk_assessment = RiskAssessment.objects.create( | ||
| name="test", | ||
| perimeter=Perimeter.objects.create(name="test", folder=test.folder), | ||
| risk_matrix=RiskMatrix.objects.all()[0], | ||
| ) | ||
| risk_assessment2 = RiskAssessment.objects.create( | ||
| name="test2", | ||
| perimeter=Perimeter.objects.create(name="test2", folder=folder), | ||
| risk_matrix=RiskMatrix.objects.all()[1], | ||
| ) | ||
| threat = Threat.objects.create(name="test", folder=test.folder) | ||
| threat2 = Threat.objects.create(name="test2", folder=folder) | ||
| asset = Asset.objects.create(name="test", folder=folder) | ||
| applied_controls = AppliedControl.objects.create(name="test", folder=folder) | ||
|
|
||
| EndpointTestsQueries.Auth.update_object( | ||
| test.client, | ||
| "Risk Scenarios", | ||
| RiskScenario, | ||
| { | ||
| "name": RISK_SCENARIO_NAME, | ||
| "description": RISK_SCENARIO_DESCRIPTION, | ||
| "ref_id": RISK_SCENARIO_REF_ID, | ||
| "existing_controls": RISK_SCENARIO_existing_controls[0], | ||
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES["value"], | ||
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT["value"], | ||
| "current_level": RISK_SCENARIO_CURRENT_LEVEL["value"], | ||
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES["value"], | ||
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT["value"], | ||
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL["value"], | ||
| "treatment": RISK_SCENARIO_TREATMENT_STATUS[0], | ||
| "justification": RISK_SCENARIO_JUSTIFICATION, | ||
| "risk_assessment": risk_assessment, | ||
| "threats": [threat], | ||
| }, | ||
| { | ||
| "name": "new " + RISK_SCENARIO_NAME, | ||
| "description": "new " + RISK_SCENARIO_DESCRIPTION, | ||
| "ref_id": "n" + RISK_SCENARIO_REF_ID, | ||
| "existing_controls": RISK_SCENARIO_existing_controls2[0], | ||
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES2["value"], | ||
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT2["value"], | ||
| "current_level": RISK_SCENARIO_CURRENT_LEVEL2["value"], | ||
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES2["value"], | ||
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT2["value"], | ||
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL2["value"], | ||
| "treatment": RISK_SCENARIO_TREATMENT_STATUS2[0], | ||
| "justification": "new " + RISK_SCENARIO_JUSTIFICATION, | ||
| "risk_assessment": str(risk_assessment2.id), | ||
| "threats": [str(threat2.id)], | ||
| "assets": [str(asset.id)], | ||
| "applied_controls": [str(applied_controls.id)], | ||
| }, | ||
| { | ||
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES, | ||
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT, | ||
| "current_level": RISK_SCENARIO_CURRENT_LEVEL, | ||
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES, | ||
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT, | ||
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL, | ||
| "treatment": RISK_SCENARIO_TREATMENT_STATUS[1], | ||
| "risk_assessment": { | ||
| "id": str(risk_assessment.id), | ||
| "str": str(risk_assessment), | ||
| "name": str(risk_assessment.name), | ||
| }, | ||
| "threats": [{"id": str(threat.id), "str": threat.name}], | ||
| "risk_matrix": { | ||
| "id": str(risk_assessment.risk_matrix.id), | ||
| "str": risk_assessment.risk_matrix.name, | ||
| }, | ||
| }, | ||
| user_group=test.user_group, | ||
| scope=str(test.folder), | ||
| fails=True) | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment the test for out-of-scope object validation.
This commented-out test is critical for validating the new functionality that ensures only relevant objects in the domain context can be used. Since the PR introduces backend validation for related objects access, having this test active would provide important coverage for that functionality.
- """def test_update_risk_scenarios_fails_with_out_of_scope_object(self, test):
+ def test_update_risk_scenarios_fails_with_out_of_scope_object(self, test):
+ """test to update risk scenarios with out-of-scope objects and expect failure"""
- """
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """def test_update_risk_scenarios_fails_with_out_of_scope_object(self, test): | |
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Risk matrix") | |
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Risk matrix2") | |
| folder = Folder.objects.create(name="test2") | |
| risk_assessment = RiskAssessment.objects.create( | |
| name="test", | |
| perimeter=Perimeter.objects.create(name="test", folder=test.folder), | |
| risk_matrix=RiskMatrix.objects.all()[0], | |
| ) | |
| risk_assessment2 = RiskAssessment.objects.create( | |
| name="test2", | |
| perimeter=Perimeter.objects.create(name="test2", folder=folder), | |
| risk_matrix=RiskMatrix.objects.all()[1], | |
| ) | |
| threat = Threat.objects.create(name="test", folder=test.folder) | |
| threat2 = Threat.objects.create(name="test2", folder=folder) | |
| asset = Asset.objects.create(name="test", folder=folder) | |
| applied_controls = AppliedControl.objects.create(name="test", folder=folder) | |
| EndpointTestsQueries.Auth.update_object( | |
| test.client, | |
| "Risk Scenarios", | |
| RiskScenario, | |
| { | |
| "name": RISK_SCENARIO_NAME, | |
| "description": RISK_SCENARIO_DESCRIPTION, | |
| "ref_id": RISK_SCENARIO_REF_ID, | |
| "existing_controls": RISK_SCENARIO_existing_controls[0], | |
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES["value"], | |
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT["value"], | |
| "current_level": RISK_SCENARIO_CURRENT_LEVEL["value"], | |
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES["value"], | |
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT["value"], | |
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL["value"], | |
| "treatment": RISK_SCENARIO_TREATMENT_STATUS[0], | |
| "justification": RISK_SCENARIO_JUSTIFICATION, | |
| "risk_assessment": risk_assessment, | |
| "threats": [threat], | |
| }, | |
| { | |
| "name": "new " + RISK_SCENARIO_NAME, | |
| "description": "new " + RISK_SCENARIO_DESCRIPTION, | |
| "ref_id": "n" + RISK_SCENARIO_REF_ID, | |
| "existing_controls": RISK_SCENARIO_existing_controls2[0], | |
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES2["value"], | |
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT2["value"], | |
| "current_level": RISK_SCENARIO_CURRENT_LEVEL2["value"], | |
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES2["value"], | |
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT2["value"], | |
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL2["value"], | |
| "treatment": RISK_SCENARIO_TREATMENT_STATUS2[0], | |
| "justification": "new " + RISK_SCENARIO_JUSTIFICATION, | |
| "risk_assessment": str(risk_assessment2.id), | |
| "threats": [str(threat2.id)], | |
| "assets": [str(asset.id)], | |
| "applied_controls": [str(applied_controls.id)], | |
| }, | |
| { | |
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES, | |
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT, | |
| "current_level": RISK_SCENARIO_CURRENT_LEVEL, | |
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES, | |
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT, | |
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL, | |
| "treatment": RISK_SCENARIO_TREATMENT_STATUS[1], | |
| "risk_assessment": { | |
| "id": str(risk_assessment.id), | |
| "str": str(risk_assessment), | |
| "name": str(risk_assessment.name), | |
| }, | |
| "threats": [{"id": str(threat.id), "str": threat.name}], | |
| "risk_matrix": { | |
| "id": str(risk_assessment.risk_matrix.id), | |
| "str": risk_assessment.risk_matrix.name, | |
| }, | |
| }, | |
| user_group=test.user_group, | |
| scope=str(test.folder), | |
| fails=True) | |
| """ | |
| def test_update_risk_scenarios_fails_with_out_of_scope_object(self, test): | |
| """test to update risk scenarios with out-of-scope objects and expect failure""" | |
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Risk matrix") | |
| EndpointTestsQueries.Auth.import_object(test.admin_client, "Risk matrix2") | |
| folder = Folder.objects.create(name="test2") | |
| risk_assessment = RiskAssessment.objects.create( | |
| name="test", | |
| perimeter=Perimeter.objects.create(name="test", folder=test.folder), | |
| risk_matrix=RiskMatrix.objects.all()[0], | |
| ) | |
| risk_assessment2 = RiskAssessment.objects.create( | |
| name="test2", | |
| perimeter=Perimeter.objects.create(name="test2", folder=folder), | |
| risk_matrix=RiskMatrix.objects.all()[1], | |
| ) | |
| threat = Threat.objects.create(name="test", folder=test.folder) | |
| threat2 = Threat.objects.create(name="test2", folder=folder) | |
| asset = Asset.objects.create(name="test", folder=folder) | |
| applied_controls = AppliedControl.objects.create(name="test", folder=folder) | |
| EndpointTestsQueries.Auth.update_object( | |
| test.client, | |
| "Risk Scenarios", | |
| RiskScenario, | |
| { | |
| "name": RISK_SCENARIO_NAME, | |
| "description": RISK_SCENARIO_DESCRIPTION, | |
| "ref_id": RISK_SCENARIO_REF_ID, | |
| "existing_controls": RISK_SCENARIO_existing_controls[0], | |
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES["value"], | |
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT["value"], | |
| "current_level": RISK_SCENARIO_CURRENT_LEVEL["value"], | |
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES["value"], | |
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT["value"], | |
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL["value"], | |
| "treatment": RISK_SCENARIO_TREATMENT_STATUS[0], | |
| "justification": RISK_SCENARIO_JUSTIFICATION, | |
| "risk_assessment": risk_assessment, | |
| "threats": [threat], | |
| }, | |
| { | |
| "name": "new " + RISK_SCENARIO_NAME, | |
| "description": "new " + RISK_SCENARIO_DESCRIPTION, | |
| "ref_id": "n" + RISK_SCENARIO_REF_ID, | |
| "existing_controls": RISK_SCENARIO_existing_controls2[0], | |
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES2["value"], | |
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT2["value"], | |
| "current_level": RISK_SCENARIO_CURRENT_LEVEL2["value"], | |
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES2["value"], | |
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT2["value"], | |
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL2["value"], | |
| "treatment": RISK_SCENARIO_TREATMENT_STATUS2[0], | |
| "justification": "new " + RISK_SCENARIO_JUSTIFICATION, | |
| "risk_assessment": str(risk_assessment2.id), | |
| "threats": [str(threat2.id)], | |
| "assets": [str(asset.id)], | |
| "applied_controls": [str(applied_controls.id)], | |
| }, | |
| { | |
| "current_proba": RISK_SCENARIO_CURRENT_PROBABILITIES, | |
| "current_impact": RISK_SCENARIO_CURRENT_IMPACT, | |
| "current_level": RISK_SCENARIO_CURRENT_LEVEL, | |
| "residual_proba": RISK_SCENARIO_RESIDUAL_PROBABILITIES, | |
| "residual_impact": RISK_SCENARIO_RESIDUAL_IMPACT, | |
| "residual_level": RISK_SCENARIO_RESIDUAL_LEVEL, | |
| "treatment": RISK_SCENARIO_TREATMENT_STATUS[1], | |
| "risk_assessment": { | |
| "id": str(risk_assessment.id), | |
| "str": str(risk_assessment), | |
| "name": str(risk_assessment.name), | |
| }, | |
| "threats": [{"id": str(threat.id), "str": threat.name}], | |
| "risk_matrix": { | |
| "id": str(risk_assessment.risk_matrix.id), | |
| "str": risk_assessment.risk_matrix.name, | |
| }, | |
| }, | |
| user_group=test.user_group, | |
| scope=str(test.folder), | |
| fails=True) |
| }, | ||
| user_group=test.user_group, | ||
| scope=str(test.folder), | ||
| fails=True) |
There was a problem hiding this comment.
Fix syntax error in the test call.
The parameter fails=True is missing a comma before it, which would cause a syntax error if the test were uncommented.
- scope=str(test.folder),
- fails=True)
+ scope=str(test.folder),
+ fails=True,
+ )📝 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.
| fails=True) | |
| scope=str(test.folder), | |
| fails=True, | |
| ) |
| accessible_ids = set( | ||
| RoleAssignment.get_accessible_object_ids( | ||
| current_folder, request.user, field.related_model | ||
| )[0] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential performance overhead in repetitive object ID retrieval.
You repeatedly call RoleAssignment.get_accessible_object_ids in a loop for each related field. If the model has many related fields, this loop might trigger multiple expensive lookups. Consider caching the computed set of accessible IDs per related model to reduce repeated work.
| EndpointTestsQueries.Auth.update_object( | ||
| test.client, | ||
| "Compliance Assessments", | ||
| ComplianceAssessment, | ||
| { | ||
| "name": COMPLIANCE_ASSESSMENT_NAME, | ||
| "description": COMPLIANCE_ASSESSMENT_DESCRIPTION, | ||
| "version": COMPLIANCE_ASSESSMENT_VERSION, | ||
| "perimeter": perimeter, | ||
| "framework": Framework.objects.all()[0], | ||
| }, | ||
| { | ||
| "name": "new " + COMPLIANCE_ASSESSMENT_NAME, | ||
| "description": "new " + COMPLIANCE_ASSESSMENT_DESCRIPTION, | ||
| "version": COMPLIANCE_ASSESSMENT_VERSION + ".1", | ||
| "perimeter": str(perimeter2.id), | ||
| "framework": str(Framework.objects.all()[1].id), | ||
| }, | ||
| { | ||
| "perimeter": { | ||
| "id": str(perimeter.id), | ||
| "str": perimeter.folder.name + "/" + perimeter.name, | ||
| "folder": { | ||
| "id": str(perimeter.folder.id), | ||
| "str": perimeter.folder.name, | ||
| }, | ||
| }, | ||
| "framework": { | ||
| "id": str(Framework.objects.all()[0].id), | ||
| "str": str(Framework.objects.all()[0]), | ||
| "implementation_groups_definition": None, | ||
| "reference_controls": [], | ||
| "min_score": Framework.objects.all()[0].min_score, | ||
| "max_score": Framework.objects.all()[0].max_score, | ||
| "ref_id": str(Framework.objects.all()[0].ref_id), | ||
| }, | ||
| }, | ||
| user_group=test.user_group, | ||
| scope=str(test.folder), | ||
| ) |
There was a problem hiding this comment.
Inconsistency in expected response structure
The test correctly updates the compliance assessment with new perimeter (perimeter2) and framework (Framework.objects.all()[1]), but the expected response structure still references the original objects.
Consider updating the expected response to match the updated values:
{
"perimeter": {
- "id": str(perimeter.id),
- "str": perimeter.folder.name + "/" + perimeter.name,
+ "id": str(perimeter2.id),
+ "str": perimeter2.folder.name + "/" + perimeter2.name,
"folder": {
- "id": str(perimeter.folder.id),
- "str": perimeter.folder.name,
+ "id": str(perimeter2.folder.id),
+ "str": perimeter2.folder.name,
},
},
"framework": {
- "id": str(Framework.objects.all()[0].id),
- "str": str(Framework.objects.all()[0]),
+ "id": str(Framework.objects.all()[1].id),
+ "str": str(Framework.objects.all()[1]),
"implementation_groups_definition": None,
"reference_controls": [],
- "min_score": Framework.objects.all()[0].min_score,
- "max_score": Framework.objects.all()[0].max_score,
- "ref_id": str(Framework.objects.all()[0].ref_id),
+ "min_score": Framework.objects.all()[1].min_score,
+ "max_score": Framework.objects.all()[1].max_score,
+ "ref_id": str(Framework.objects.all()[1].ref_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.
| EndpointTestsQueries.Auth.update_object( | |
| test.client, | |
| "Compliance Assessments", | |
| ComplianceAssessment, | |
| { | |
| "name": COMPLIANCE_ASSESSMENT_NAME, | |
| "description": COMPLIANCE_ASSESSMENT_DESCRIPTION, | |
| "version": COMPLIANCE_ASSESSMENT_VERSION, | |
| "perimeter": perimeter, | |
| "framework": Framework.objects.all()[0], | |
| }, | |
| { | |
| "name": "new " + COMPLIANCE_ASSESSMENT_NAME, | |
| "description": "new " + COMPLIANCE_ASSESSMENT_DESCRIPTION, | |
| "version": COMPLIANCE_ASSESSMENT_VERSION + ".1", | |
| "perimeter": str(perimeter2.id), | |
| "framework": str(Framework.objects.all()[1].id), | |
| }, | |
| { | |
| "perimeter": { | |
| "id": str(perimeter.id), | |
| "str": perimeter.folder.name + "/" + perimeter.name, | |
| "folder": { | |
| "id": str(perimeter.folder.id), | |
| "str": perimeter.folder.name, | |
| }, | |
| }, | |
| "framework": { | |
| "id": str(Framework.objects.all()[0].id), | |
| "str": str(Framework.objects.all()[0]), | |
| "implementation_groups_definition": None, | |
| "reference_controls": [], | |
| "min_score": Framework.objects.all()[0].min_score, | |
| "max_score": Framework.objects.all()[0].max_score, | |
| "ref_id": str(Framework.objects.all()[0].ref_id), | |
| }, | |
| }, | |
| user_group=test.user_group, | |
| scope=str(test.folder), | |
| ) | |
| EndpointTestsQueries.Auth.update_object( | |
| test.client, | |
| "Compliance Assessments", | |
| ComplianceAssessment, | |
| { | |
| "name": COMPLIANCE_ASSESSMENT_NAME, | |
| "description": COMPLIANCE_ASSESSMENT_DESCRIPTION, | |
| "version": COMPLIANCE_ASSESSMENT_VERSION, | |
| "perimeter": perimeter, | |
| "framework": Framework.objects.all()[0], | |
| }, | |
| { | |
| "name": "new " + COMPLIANCE_ASSESSMENT_NAME, | |
| "description": "new " + COMPLIANCE_ASSESSMENT_DESCRIPTION, | |
| "version": COMPLIANCE_ASSESSMENT_VERSION + ".1", | |
| "perimeter": str(perimeter2.id), | |
| "framework": str(Framework.objects.all()[1].id), | |
| }, | |
| { | |
| "perimeter": { | |
| "id": str(perimeter2.id), | |
| "str": perimeter2.folder.name + "/" + perimeter2.name, | |
| "folder": { | |
| "id": str(perimeter2.folder.id), | |
| "str": perimeter2.folder.name, | |
| }, | |
| }, | |
| "framework": { | |
| "id": str(Framework.objects.all()[1].id), | |
| "str": str(Framework.objects.all()[1]), | |
| "implementation_groups_definition": None, | |
| "reference_controls": [], | |
| "min_score": Framework.objects.all()[1].min_score, | |
| "max_score": Framework.objects.all()[1].max_score, | |
| "ref_id": str(Framework.objects.all()[1].ref_id), | |
| }, | |
| }, | |
| user_group=test.user_group, | |
| scope=str(test.folder), | |
| ) |
we should not be proposed all the objects that we can see, but only the ones that are relevant for the context (domain) in which we are editing.
Backend function checking for this is created and applyu to create and edit objects functions.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation