fix: enforce permissions for domain import #1400
Conversation
WalkthroughThe pull request introduces security enhancements to the domain import functionality across backend and frontend components. Key changes include implementing robust permission checks in the 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 (
|
nas-tabchiche
changed the title
nas-tabchiche
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/views.py(5 hunks)frontend/messages/en.json(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts(2 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: ruff (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
backend/core/views.py (1)
70-70: ImportingPermissionDeniedexceptionThe import of
PermissionDeniedfromrest_framework.exceptionsis appropriate for handling permission errors in the API views.frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts (4)
98-98: Preserve form data withwithFileson validation failureUpdating the failure response to use
withFiles({ form })ensures that form data, including files, is preserved when validation fails. This enhances user experience during form submission.
121-121: Preserve form data withwithFileswhen missing libraries are detectedBy returning
fail(400, withFiles({ form })), the form data is preserved when missing libraries are detected during import, allowing the user to correct the issue without losing their inputs.
127-127: Preserve form data after successful import withwithFilesReturning
withFiles({ form })ensures that the form data is preserved even after a successful import, which may be beneficial if you want to reset or maintain form state.
132-132: Preserve form data withwithFileson general API errorsUsing
fail(400, withFiles({ form }))when a general API error occurs helps to preserve form data, allowing users to retry submission without re-uploading files.frontend/messages/en.json (1)
648-648: Addition of error message for unauthorized domain importThe new message
"userDoesNotHavePermissionToImportDomain"provides a clear and localized error message informing users when they lack permission to import a domain.
nas-tabchiche
force-pushed
the
fix/enforce-permissions-for-domain-import
branch
from
4330d47 to
ac2d5c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/core/views.py (1)
2219-2226:⚠️ Potential issueUse
has_permmethod to check permissions in object import.Directly accessing
user.permissionsmay not accurately represent the user's permissions. Instead, useuser.has_perm(f'{model._meta.app_label}.add_{model._meta.model_name}')to check if the user has the required permissions for each model.Apply this diff to fix the permission checks:
- for model in models_map.values(): - if f"add_{model._meta.model_name}" not in user.permissions: - error_dict[model._meta.model_name] = "permission_denied" + for model in models_map.values(): + perm_codename = f"{model._meta.app_label}.add_{model._meta.model_name}" + if not user.has_perm(perm_codename): + error_dict[model._meta.model_name] = "permission_denied"
🧹 Nitpick comments (1)
backend/core/views.py (1)
2107-2114: Remove unused exception variables.The exception variables
eare assigned but never used in the error handling blocks.Apply this diff to remove the unused variables:
- except KeyError as e: + except KeyError: logger.error("No file provided in the request", exc_info=True) return Response( {"errors": ["No file provided"]}, status=status.HTTP_400_BAD_REQUEST ) - except json.JSONDecodeError as e: + except json.JSONDecodeError: logger.error("Invalid JSON format in uploaded file", exc_info=True) return Response( {"errors": ["Invalid JSON format"]}, status=status.HTTP_400_BAD_REQUEST )🧰 Tools
🪛 Ruff (0.8.2)
2107-2107: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
2113-2113: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/views.py(8 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/views.py
2096-2096: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
2107-2107: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
2113-2113: Local variable e is assigned to but never used
Remove assignment to unused variable e
(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: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/core/views.py (2)
2085-2088:⚠️ Potential issueFix incorrect permission check.
The permission check is using direct access to
request.user.permissionswhich may not accurately reflect the user's permissions. Use Django's built-in permission system instead.Apply this diff to fix the permission check:
-if "add_folder" not in request.user.permissions: +if not request.user.has_perm("iam.add_folder"):Likely invalid or redundant comment.
2096-2105:⚠️ Potential issueFix incorrect logger usage.
The
exc_infoparameter in the logger call should be a boolean flag to include the stack trace, not the exception object.Apply this diff to fix the logger call:
- exc_info=True, + exc_info=e,Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
2096-2096: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/core/views.py (2)
2107-2114: Enhance error logging for better debugging.While the error handling is good, consider adding
exc_info=Trueto capture the full stack trace in the logs:- logger.error("No file provided in the request", exc_info=True) + logger.error("No file provided in the request", exc_info=True) - logger.error("Invalid JSON format in uploaded file", exc_info=True) + logger.error("Invalid JSON format in uploaded file", exc_info=True)
2282-2284: Use exception chaining to preserve error context.To maintain the exception context and improve debugging, use explicit exception chaining:
- raise ValidationError({"missing_libraries": missing_libraries}) + raise ValidationError({"missing_libraries": missing_libraries}) from NoneThis change helps distinguish between errors in the validation and errors in exception handling.
🧰 Tools
🪛 Ruff (0.8.2)
2283-2283: 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(11 hunks)
🧰 Additional context used
📓 Learnings (1)
backend/core/views.py (1)
Learnt from: nas-tabchiche
PR: intuitem/ciso-assistant-community#1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
🪛 Ruff (0.8.2)
backend/core/views.py
2283-2283: 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-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: enterprise-startup-docker-compose-test
🔇 Additional comments (2)
backend/core/views.py (2)
2085-2106: LGTM! Permission check implementation is secure and well-structured.The permission check correctly uses the custom permission system through
user.permissionsand provides appropriate error handling with logging.
2219-2226: LGTM! Comprehensive permission validation for all object types.The implementation correctly validates that the user has create permissions for all object types before proceeding with the import, which is a robust security measure.
Docstrings generation was requested by @nas-tabchiche. * #1400 (comment) The following files were modified: * `backend/core/views.py`
|
Note Generated docstrings for this pull request, at #1402 |
nas-tabchiche
requested review from
ab-smith,
eric-intuitem and
Mohamed-Hacene
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/core/views.py (2)
2111-2119: Enhance error handling with more specific messages and proper exception chaining.While the error handling is good, consider these improvements:
- Make the KeyError message more specific: "No file provided in the request" → "Missing 'file' key in request data"
- Use exception chaining to preserve the traceback.
Apply this diff to improve the error handling:
- except KeyError: - logger.error("No file provided in the request", exc_info=True) + except KeyError as e: + logger.error("Missing 'file' key in request data", exc_info=True) return Response( - {"errors": ["No file provided"]}, status=status.HTTP_400_BAD_REQUEST + {"errors": [f"Missing required field: {e}"]}, status=status.HTTP_400_BAD_REQUEST ) - except json.JSONDecodeError: - logger.error("Invalid JSON format in uploaded file", exc_info=True) + except json.JSONDecodeError as e: + logger.error("Invalid JSON format in uploaded file", exc_info=True) return Response( - {"errors": ["Invalid JSON format"]}, status=status.HTTP_400_BAD_REQUEST + {"errors": [f"Invalid JSON format: {e}"]}, status=status.HTTP_400_BAD_REQUEST )
Line range hint
2445-2449: Improve exception handling with proper exception chaining.The error handling should preserve the original exception's context using the
raise ... from errsyntax.Apply this diff to improve the exception handling:
- logger.error("Error creating object", obj=obj, exc_info=True) - # This will trigger a rollback of the entire batch - raise ValidationError( - f"Error creating {model._meta.model_name}: {str(e)}" - ) + logger.error("Error creating object", obj=obj, exc_info=True) + # This will trigger a rollback of the entire batch + raise ValidationError( + f"Error creating {model._meta.model_name}: {str(e)}" + ) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(11 hunks)
🧰 Additional context used
📓 Learnings (1)
backend/core/views.py (1)
Learnt from: nas-tabchiche
PR: intuitem/ciso-assistant-community#1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
🪛 Ruff (0.8.2)
backend/core/views.py
2293-2293: 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 (5)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
🔇 Additional comments (2)
backend/core/views.py (2)
2085-2109: LGTM! Well-implemented permission checks.The permission check implementation is robust, with proper error handling and logging. The code correctly verifies the user's permission to add folders before proceeding with the domain import.
2223-2236: LGTM! Comprehensive permission validation for object creation.The implementation thoroughly validates permissions for each model type before proceeding with the import. The error handling is well-structured, and the code maintains good security practices.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/core/views.py (1)
2115-2125:⚠️ Potential issueFix the usage of
exc_infoparameter in logger.error.When logging exceptions, the
exc_infoparameter should be a boolean flag indicating whether to include the stack trace.- logger.error( - "User does not have permission to import domain", - user=request.user, - exc_info=True, - ) + logger.error( + "User does not have permission to import domain", + user=request.user, + exc_info=True, + )
🧹 Nitpick comments (1)
backend/core/views.py (1)
2006-2020: LGTM! Comprehensive permission check implementation.The permission check implementation is thorough, ensuring the user has view permissions for all object types in the domain before allowing export.
Minor optimization: Use
key in dictinstead ofkey in dict.keys()For better performance, remove the
.keys()call as it creates an unnecessary view object.-for model in objects.keys(): +for model in objects:🧰 Tools
🪛 Ruff (0.8.2)
2006-2006: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(12 hunks)
🧰 Additional context used
📓 Learnings (1)
backend/core/views.py (1)
Learnt from: nas-tabchiche
PR: intuitem/ciso-assistant-community#1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
🪛 Ruff (0.8.2)
backend/core/views.py
2006-2006: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
2308-2308: 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-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- 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: build (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/core/views.py (2)
2238-2251: LGTM! Robust permission validation for object creation.The implementation correctly validates that the user has permission to create all object types before proceeding with the import. The error handling with the error dictionary is well-structured.
2367-2367: LGTM! Consistent error logging implementation.The error logging is well-implemented with proper use of the exc_info parameter for stack trace inclusion.
Also applies to: 2460-2460
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/core/views.py (5)
2006-2006: Optimize dictionary key lookup.Use
key in objectsinstead ofkey in objects.keys()for better performance.- for model in objects.keys(): + for model in objects:🧰 Tools
🪛 Ruff (0.8.2)
2006-2006: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
2192-2193: Enhance attachment extraction error handling.The generic exception catch block should log more details about the failed attachment.
- except Exception: - logger.error("Error extracting attachment", exc_info=True) + except Exception as e: + logger.error( + "Error extracting attachment", + attachment=current_name, + error=str(e), + exc_info=True + )
2241-2243: Add type hints for better code clarity.The code uses a conditional assignment that could benefit from type hints.
+ from core.models import ComplianceAssessment model2 = ( ComplianceAssessment if model == RequirementAssessment else model )🧰 Tools
🪛 Ruff (0.8.2)
2242-2242:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
Line range hint
2354-2369: Enhance validation error logging.Add more context to validation error logs to help with debugging.
if not serializer.is_valid(): + logger.error( + "Validation failed", + model=model_name, + object_id=obj_id, + errors=serializer.errors + ) validation_errors.append( { "model": model_name, "id": obj_id, "errors": serializer.errors, } )
Line range hint
2462-2466: Improve error message detail.Include more context in the error message to help identify the specific object that failed to create.
- logger.error("Error creating object", obj=obj, exc_info=True) + logger.error( + "Error creating object", + model=model._meta.model_name, + object_id=obj_id, + fields=fields, + exc_info=True + ) raise ValidationError( - f"Error creating {model._meta.model_name}: {str(e)}" + f"Error creating {model._meta.model_name} (ID: {obj_id}): {str(e)}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(12 hunks)
🧰 Additional context used
📓 Learnings (1)
backend/core/views.py (1)
Learnt from: nas-tabchiche
PR: intuitem/ciso-assistant-community#1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
🪛 Ruff (0.8.2)
backend/core/views.py
2006-2006: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
2242-2242: RequirementAssessment may be undefined, or defined from star imports
(F405)
2310-2310: 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 (2)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/core/views.py (1)
2116-2120: 🛠️ Refactor suggestionImprove error logging.
The
exc_info=Trueparameter is incorrectly passed directly to the error message. Instead, it should be a separate parameter to include the stack trace.- user=request.user, - exc_info=True, + user=request.user, + exc_info=TrueLikely invalid or redundant comment.
| logger.warning("Missing libraries", libraries=missing_libraries) | ||
| raise ValidationError({"missing_libraries": missing_libraries}) | ||
| logger.exception(f"Failed to import objects: {str(e)}") | ||
| logger.exception("Failed to import objects", objects=str(e)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve exception context in error handling.
Use raise ... from None to explicitly suppress the original exception context.
- raise ValidationError({"missing_libraries": missing_libraries})
+ raise ValidationError({"missing_libraries": missing_libraries}) from None📝 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.
| logger.warning("Missing libraries", libraries=missing_libraries) | |
| raise ValidationError({"missing_libraries": missing_libraries}) | |
| logger.exception(f"Failed to import objects: {str(e)}") | |
| logger.exception("Failed to import objects", objects=str(e)) | |
| logger.warning("Missing libraries", libraries=missing_libraries) | |
| raise ValidationError({"missing_libraries": missing_libraries}) from None | |
| logger.exception("Failed to import objects", objects=str(e)) |
🧰 Tools
🪛 Ruff (0.8.2)
2310-2310: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
nas-tabchiche
force-pushed
the
fix/enforce-permissions-for-domain-import
branch
from
c629855 to
8834c09
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/core/views.py (2)
Line range hint
2245-2281: Improve error handling in attachment extraction.The error handling for attachment extraction could be more specific about what went wrong.
- except Exception: - logger.error("Error extracting attachment", exc_info=True) + except Exception as e: + logger.error( + "Error extracting attachment", + attachment=current_name, + error=str(e), + exc_info=True + )🧰 Tools
🪛 Ruff (0.8.2)
2250-2250: Use
import_version != VERSIONinstead ofnot import_version == VERSIONReplace with
!=operator(SIM201)
2409-2411: Preserve exception context in error handling.Use
raise ... from Noneto explicitly suppress the original exception context.- raise ValidationError({"missing_libraries": missing_libraries}) + raise ValidationError({"missing_libraries": missing_libraries}) from None🧰 Tools
🪛 Ruff (0.8.2)
2410-2410: 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 (4)
backend/core/views.py(12 hunks)frontend/messages/en.json(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts(2 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/messages/en.json
- frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte
- frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts
🧰 Additional context used
📓 Learnings (1)
backend/core/views.py (1)
Learnt from: nas-tabchiche
PR: intuitem/ciso-assistant-community#1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
🪛 Ruff (0.8.2)
backend/core/views.py
107-107: Redefinition of unused PermissionDenied from line 68
(F811)
2089-2089: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
2410-2410: 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-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
backend/core/views.py (6)
2089-2103: LGTM! Comprehensive permission checks for domain export.The implementation correctly verifies that the user has view permissions for all object types in the domain before allowing export. Good error handling with appropriate logging.
🧰 Tools
🪛 Ruff (0.8.2)
2089-2089: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
2188-2213: LGTM! Proper permission check for domain import.The implementation correctly verifies that the user has permission to add folders before allowing domain import.
2214-2221: LGTM! Clear error handling for missing files.Good error handling with appropriate error messages and logging.
2328-2341: LGTM! Thorough permission checks before object creation.The implementation correctly verifies that the user has permission to create all object types before proceeding with the import. Good use of error dictionary to collect all permission issues.
Line range hint
2456-2471: LGTM! Comprehensive validation error handling.Good error handling with appropriate context and logging.
Line range hint
2564-2568: LGTM! Proper error handling in object creation.Good error handling with appropriate logging and clear error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/views.py (1)
2278-2279: Improve error handling for attachment extraction.The current implementation silently continues after logging attachment extraction errors. Consider notifying the user about failed attachments or collecting them in a list for a summary report.
Apply this diff to improve error handling:
+ failed_attachments = [] try: content = zipf.read(attachment) current_name = Path(attachment.filename).name new_name = default_storage.save( current_name, io.BytesIO(content) ) if new_name != current_name: for x in json_dump["objects"]: if ( x["model"] == "core.evidence" and x["fields"]["attachment"] == current_name ): x["fields"]["attachment"] = new_name - except Exception: - logger.error("Error extracting attachment", exc_info=True) + except Exception as err: + logger.error("Error extracting attachment", exc_info=True) + failed_attachments.append((attachment.filename, str(err))) + + if failed_attachments: + logger.warning("Failed to extract attachments", attachments=failed_attachments) + raise ValidationError({"failed_attachments": failed_attachments})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(11 hunks)
🧰 Additional context used
📓 Learnings (1)
backend/core/views.py (1)
Learnt from: nas-tabchiche
PR: intuitem/ciso-assistant-community#1400
File: backend/core/views.py:2219-2226
Timestamp: 2025-01-22T15:55:06.417Z
Learning: The codebase uses a custom permission system through `RoleAssignment.get_permissions()` accessed via `user.permissions` property, instead of Django's default permission system.
🪛 Ruff (0.8.2)
backend/core/views.py
2087-2087: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
2408-2408: 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 (8)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
🔇 Additional comments (6)
backend/core/views.py (6)
2087-2101: LGTM! Comprehensive permission checks for domain export.The implementation correctly enforces view permissions for each model type in the domain export using the custom permission system.
🧰 Tools
🪛 Ruff (0.8.2)
2087-2087: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
2186-2211: LGTM! Robust permission checks and error handling for domain import.The implementation correctly enforces the 'add_folder' permission and includes comprehensive error handling with appropriate logging.
2326-2339: LGTM! Comprehensive permission checks for object creation during import.The implementation correctly enforces 'add' permissions for each model type and provides clear error information.
Line range hint
2562-2566: LGTM! Clear error handling for object creation failures.The implementation provides detailed error information by including both the model name and the original error message.
2407-2409: 🛠️ Refactor suggestionPreserve exception context in validation error handling.
When re-raising exceptions within an except clause, use
raise ... from Noneto explicitly suppress the exception context.Apply this diff to fix the error handling:
- raise ValidationError({"missing_libraries": missing_libraries}) + raise ValidationError({"missing_libraries": missing_libraries}) from NoneLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
2408-2408: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
2243-2246: 🛠️ Refactor suggestionPreserve exception context in error handling.
When re-raising exceptions within an except clause, use
raise ... from errorraise ... from Noneto preserve or explicitly suppress the exception context.Apply this diff to fix the error handling:
- except json.JSONDecodeError: - logger.error("Invalid JSON format in uploaded file", exc_info=True) - raise + except json.JSONDecodeError as err: + logger.error("Invalid JSON format in uploaded file", exc_info=True) + raise err from errLikely invalid or redundant comment.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
The release focuses on strengthening permission management and refining user interface elements during domain import processes.