feat(j-s): Connect case files to defendant or civil claimants #17043
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (8)
apps/judicial-system/api/src/app/modules/file/dto/createFile.input.ts (1)
74-82: Add ID format validation.Consider adding validation to ensure the ID fields follow the expected format. This will help catch invalid IDs early in the request lifecycle.
Here's the suggested implementation:
@Allow() @IsOptional() + @Matches(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/, { + message: 'Invalid UUID format for defendantId', + }) @Field(() => ID, { nullable: true }) readonly defendantId?: string @Allow() @IsOptional() + @Matches(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/, { + message: 'Invalid UUID format for civilClaimantId', + }) @Field(() => ID, { nullable: true }) readonly civilClaimantId?: stringapps/judicial-system/backend/src/app/modules/file/models/file.model.ts (2)
49-58: Consider adding mutual exclusivity validationThe foreign key implementation looks good, but since a case file should be connected to either a defendant or a civil claimant (not both), consider adding validation to ensure mutual exclusivity.
Consider adding a validator or constraint. Here's a suggested implementation:
+ @BeforeValidate + static validateAssociations(instance: CaseFile) { + if (instance.defendantId && instance.civilClaimantId) { + throw new Error('A case file cannot be associated with both a defendant and a civil claimant'); + } + }
Line range hint
16-16: Address the TODO comment about index file importsThe comment suggests there's a pending task to improve the import structure. This would be a good opportunity to address it while making these changes.
Would you like me to help create an index file for better import organization?
libs/judicial-system/types/src/lib/notification.ts (2)
Line range hint
8-8: Remove commented code indicating future removalThe comment
// Will be removedsuggests thatADVOCATE_ASSIGNEDshould be removed, but it's still present in bothCaseNotificationTypeandNotificationTypeenums. Either remove the entry completely or remove the comment if the entry should be kept.🧰 Tools
🪛 Biome (1.9.4)
[error] 71-71: Shouldn't redeclare 'INDICTMENT_SENT_TO_PRISON_ADMIN'. Consider to delete it or rename it.
'INDICTMENT_SENT_TO_PRISON_ADMIN' is defined here:
(lint/suspicious/noRedeclare)
[error] 72-72: Shouldn't redeclare 'INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN'. Consider to delete it or rename it.
'INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN' is defined here:
(lint/suspicious/noRedeclare)
Based on the verification results, I can now provide a final response:
Notification type changes require attention in backend services
The verification reveals:
ADVOCATE_ASSIGNEDis actively used in the case notification service for handling advocate assignment notificationsINDICTMENT_SENT_TO_PRISON_ADMINandINDICTMENT_WITHDRAWN_FROM_PRISON_ADMINare used in:
- Defendant notification service for sending notifications
- Defendant service for message type determination
- Multiple test files validating this functionality
The comment about removing
ADVOCATE_ASSIGNEDneeds to be addressed as it's still being used in the notification handling logic.🔗 Analysis chain
Line range hint
1-85: Verify the impact of notification type changesThe changes to notification types might affect notification handling across the system. Let's verify the usage of these notification types.
Let me generate additional verification scripts to check for any frontend usage of these notification types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of notification types in handlers and subscribers # Check for notification handlers rg -A 5 "case.*ADVOCATE_ASSIGNED.*:" apps/judicial-system/ # Check for notification subscribers rg -A 5 "INDICTMENT_SENT_TO_PRISON_ADMIN|INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN" apps/judicial-system/Length of output: 9125
Script:
#!/bin/bash # Check frontend usage of notification types rg -A 5 "ADVOCATE_ASSIGNED|INDICTMENT_SENT_TO_PRISON_ADMIN|INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN" apps/judicial-system/web/ # Check for any GraphQL or API schema definitions rg -A 5 "ADVOCATE_ASSIGNED|INDICTMENT_SENT_TO_PRISON_ADMIN|INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN" "*.graphql" # Check for any test files using these notification types rg -A 5 "ADVOCATE_ASSIGNED|INDICTMENT_SENT_TO_PRISON_ADMIN|INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN" "*.spec.ts" "*.test.ts"Length of output: 514
🧰 Tools
🪛 Biome (1.9.4)
[error] 71-71: Shouldn't redeclare 'INDICTMENT_SENT_TO_PRISON_ADMIN'. Consider to delete it or rename it.
'INDICTMENT_SENT_TO_PRISON_ADMIN' is defined here:
(lint/suspicious/noRedeclare)
[error] 72-72: Shouldn't redeclare 'INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN'. Consider to delete it or rename it.
'INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN' is defined here:
(lint/suspicious/noRedeclare)
apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx (2)
71-79: Consider enhancing error handling and user feedbackWhile the upload result handling is functional, consider these improvements:
- Add more specific error messages based on the failure reason
- Show a loading state during the upload process
- Consider adding retry functionality for failed uploads
const uploadResult = await handleUpload( uploadFiles.filter((file) => file.percent === 0), updateUploadFile, ) if (uploadResult !== 'ALL_SUCCEEDED') { - setUploadFileError(formatMessage(errors.uploadFailed)) + setUploadFileError(formatMessage( + uploadResult === 'PARTIAL_SUCCESS' + ? errors.uploadPartiallyFailed + : errors.uploadFailed + )) return }
137-137: Consider adding file validation constraintsThe file upload implementation could benefit from additional validation:
- Add maximum file size limit
- Set maximum number of files allowed
- Add validation for minimum file size to prevent empty files
<InputFileUpload fileList={uploadFiles.filter( (file) => file.category === CaseFileCategory.SENT_TO_PRISON_ADMIN_FILE, )} accept="application/pdf" + maxFileSize={10 * 1024 * 1024} // 10MB + maxFiles={5} header={formatMessage(core.uploadBoxTitle)} description={formatMessage(core.uploadBoxDescription, { fileEndings: '.pdf', })} buttonLabel={formatMessage(core.uploadBoxButtonLabel)} onChange={handleFileUpload} onRemove={handleRemoveFile} />apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (1)
128-147: Consider enhancing clarity and documentationWhile the implementation is functionally correct, consider these improvements:
- Make the undefined defendant parameter more explicit in the createCaseFile call
- Add more detailed API documentation including:
- Required permissions
- Expected request/response formats
- Possible error scenarios
Consider this enhancement:
@ApiCreatedResponse({ type: CaseFile, description: 'Creates a new case file for a civil claimant', + summary: 'Create a case file associated with a civil claimant', + schema: { + example: { + // Add example response + } + } }) + @ApiBody({ type: CreateFileDto }) + @ApiBadRequestResponse({ description: 'Invalid input' }) + @ApiUnauthorizedResponse({ description: 'Unauthorized' }) + @ApiForbiddenResponse({ description: 'Forbidden' }) createCivilClaimantCaseFile( @Param('caseId') caseId: string, @Param('civilClaimantId') civilClaimantId: string, @CurrentHttpUser() user: User, @CurrentCase() theCase: Case, @CurrentCivilClaimant() civilClaimant: CivilClaimant, @Body() createFile: CreateFileDto, ): Promise<CaseFile> { this.logger.debug( `Creating a file for case ${caseId} and civil claimant ${civilClaimantId}`, ) return this.fileService.createCaseFile( theCase, createFile, user, - undefined, + null, // Explicitly indicate no defendant civilClaimant, ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
apps/judicial-system/api/src/app/modules/backend/backend.service.ts(2 hunks)apps/judicial-system/api/src/app/modules/file/dto/createFile.input.ts(1 hunks)apps/judicial-system/api/src/app/modules/file/file.resolver.ts(1 hunks)apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts(1 hunks)apps/judicial-system/backend/migrations/20241122091513-update-case-file.js(1 hunks)apps/judicial-system/backend/src/app/modules/file/file.controller.ts(3 hunks)apps/judicial-system/backend/src/app/modules/file/file.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessWriteCaseFileGuard.spec.ts(5 hunks)apps/judicial-system/backend/src/app/modules/file/guards/writeCaseFile.guard.ts(1 hunks)apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts(2 hunks)apps/judicial-system/backend/src/app/modules/file/models/file.model.ts(2 hunks)apps/judicial-system/backend/src/app/modules/file/test/fileController/createCaseFileGuards.spec.ts(3 hunks)apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx(4 hunks)apps/judicial-system/web/src/utils/hooks/useS3Upload/useS3Upload.ts(3 hunks)libs/judicial-system/types/src/lib/notification.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessWriteCaseFileGuard.spec.ts
🧰 Additional context used
📓 Path-based instructions (14)
apps/judicial-system/api/src/app/modules/backend/backend.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/src/app/modules/file/dto/createFile.input.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/src/app/modules/file/file.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/migrations/20241122091513-update-case-file.js (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/file.controller.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/file.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/guards/writeCaseFile.guard.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/models/file.model.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/test/fileController/createCaseFileGuards.spec.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/utils/hooks/useS3Upload/useS3Upload.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/judicial-system/types/src/lib/notification.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
📓 Learnings (4)
apps/judicial-system/backend/migrations/20241122091513-update-case-file.js (2)
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/backend/migrations/20241022142128-update-defendant.js:18-24
Timestamp: 2024-11-12T15:15:20.157Z
Learning: When rolling back migrations, if the associated code that uses the fields is also rolled back, data preservation concerns may not be an issue.
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/backend/migrations/20241022142128-update-defendant.js:1-2
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the 'judicial-system' application, it is a convention to include `'use strict'` directives at the top of migration files in `apps/judicial-system/backend/migrations/*.js`.
apps/judicial-system/backend/src/app/modules/file/test/fileController/createCaseFileGuards.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx:68-78
Timestamp: 2024-11-19T09:16:52.661Z
Learning: In the `SendToPrisonAdmin` component (`apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx`), the file upload functionality is currently not being implemented, so updating the defendant's status can proceed without waiting for the file upload.
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx:57-82
Timestamp: 2024-11-19T09:52:19.705Z
Learning: In the `SendToPrisonAdmin` component (`apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx`), error handling is handled within the `updateDefendant` function, so additional try-catch blocks in the calling function are unnecessary.
libs/judicial-system/types/src/lib/notification.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16556
File: apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.ts:13-16
Timestamp: 2024-11-12T15:15:26.274Z
Learning: In the `CaseNotificationType` enum in `libs/judicial-system/types/src/lib/notification.ts`, `APPEAL_CASE_FILES_UPDATED` and `CASE_FILES_UPDATED` are distinct notification types that need to remain separate, as they serve different purposes in the notification system.
🪛 Biome (1.9.4)
apps/judicial-system/backend/migrations/20241122091513-update-case-file.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
libs/judicial-system/types/src/lib/notification.ts
[error] 71-71: Shouldn't redeclare 'INDICTMENT_SENT_TO_PRISON_ADMIN'. Consider to delete it or rename it.
'INDICTMENT_SENT_TO_PRISON_ADMIN' is defined here:
(lint/suspicious/noRedeclare)
[error] 72-72: Shouldn't redeclare 'INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN'. Consider to delete it or rename it.
'INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (23)
apps/judicial-system/backend/src/app/modules/file/file.controller.ts (4)
52-59: Added necessary imports for defendants and civil claimants
The imports for Defendant, DefendantExistsGuard, CurrentDefendant, CivilClaimant, CivilClaimantExistsGuard, and CurrentCivilClaimant are appropriately added to support the new functionalities.
66-66: Importing WriteCaseFileGuard for enhanced security
Including WriteCaseFileGuard strengthens security by ensuring that file write operations are properly authorized.
110-110: Added WriteCaseFileGuard to createCaseFile method
Adding WriteCaseFileGuard to the createCaseFile method's guards ensures that only authorized users can create case files, enhancing security.
138-164: New method createDefendantCaseFile implemented correctly
The createDefendantCaseFile method correctly facilitates creating case files associated with a defendant, with appropriate guards and role restrictions. The use of DefendantExistsGuard and WriteCaseFileGuard ensures proper validation and authorization.
apps/judicial-system/backend/migrations/20241122091513-update-case-file.js (3)
1-2: LGTM: Keeping 'use strict' directive per project convention.
While modern JavaScript modules are automatically strict, we'll maintain the directive as it's an established convention in the judicial-system migrations.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
4-43: LGTM: Well-structured migration with proper transaction handling.
The migration is well implemented with:
- Proper use of transactions for atomicity
- Correct foreign key references
- Complete rollback functionality
- Appropriate ordering of operations in both up and down migrations
9-16: Verify the business rules for column nullability.
Both defendant_id and civil_claimant_id are defined as nullable. Please verify if there should be a business rule ensuring that at least one of these fields must be populated (i.e., a case file must be associated with either a defendant or a civil claimant).
Also applies to: 22-29
✅ Verification successful
Let me gather more information about the case file model and any existing validation rules.
Let me search for any business rules or validations specifically related to case files and their associations.
Let me search for any model definitions or service layer code that might contain business rules.
Based on the code analysis, I can now provide a final response:
The nullable fields are correctly defined - no additional validation rules needed.
The case file model allows both defendant_id and civil_claimant_id to be nullable by design, as evidenced by:
- The migration explicitly sets
allowNull: truefor both fields - The codebase shows separate endpoints and flows for handling defendant and civil claimant files
- The file service and controllers demonstrate that files can be associated with either a defendant or a civil claimant independently
- The system architecture supports different types of case files that may be relevant to only one party
This flexibility in the data model is intentional to support various types of legal documents that may pertain to either a defendant or a civil claimant, but not necessarily both.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing validation rules or constraints
# Look for related model validations
ast-grep --pattern 'class CaseFile {
$$$
@$_
$_Id: $_
$$$
}'
# Search for any validation decorators or constraints
rg -A 5 "validateCaseFile|@Validate|@Check"
Length of output: 29417
Script:
#!/bin/bash
# Search for case file model and validation rules
ast-grep --pattern 'class CaseFile {
$$$
}'
# Search for any validation rules or constraints related to case files
rg -A 5 "CaseFile.*validate|@Check.*CaseFile|@Validate.*CaseFile"
# Look for any business logic or service layer validations
rg -A 5 "createCase|updateCase|validateCase"
Length of output: 124783
Script:
#!/bin/bash
# Search for case file related validations and business rules
rg -A 5 "defendant_id|civil_claimant_id" apps/judicial-system/backend/
# Look for any database constraints or checks
rg -A 5 "CHECK.*defendant_id|CHECK.*civil_claimant_id" apps/judicial-system/backend/
# Search for any service layer validations
rg -A 5 "validateCaseFile|validateCase" apps/judicial-system/backend/
Length of output: 3619
Script:
#!/bin/bash
# Search for case file model definition
rg -A 20 "class CaseFile|interface CaseFile" apps/judicial-system/backend/
# Look for any service layer code handling case files
rg -A 10 "createCaseFile|updateCaseFile" apps/judicial-system/backend/
# Search for any validation decorators or constraints in the models
ast-grep --pattern 'class CaseFile {
$$$
@$_
$_: $_
$$$
}'
Length of output: 84503
apps/judicial-system/backend/src/app/modules/file/test/fileController/createCaseFileGuards.spec.ts (2)
7-7: LGTM! Import statement is correctly placed
The WriteCaseFileGuard import follows the established import organization pattern.
21-21: LGTM! Test expectation updated correctly
The test expectation has been properly updated to reflect the addition of the new WriteCaseFileGuard.
apps/judicial-system/backend/src/app/modules/file/models/file.model.ts (1)
20-20: Verify the module structure for CivilClaimant model
The CivilClaimant model is imported from the defendant directory, which seems counterintuitive. Consider moving it to a more appropriate module or directory that better reflects its domain.
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts (1)
67-78: Verify architectural consistency across resolvers
The addition of civilClaimantId in this resolver suggests a pattern that might need to be applied consistently across other resolvers handling case files.
apps/judicial-system/api/src/app/modules/file/file.resolver.ts (2)
68-68: LGTM: Clean input destructuring
The addition of defendantId and civilClaimantId follows TypeScript best practices and maintains type safety.
75-80: Consider adding parameter validation
While the implementation is correct, consider adding validation for the new parameters to ensure they are properly handled when either defendantId or civilClaimantId is provided but invalid.
Let's verify if validation exists in the backend service:
Consider adding validation like this:
if (defendantId) {
// Verify defendant exists and has proper access
await this.validateDefendant(defendantId);
}
if (civilClaimantId) {
// Verify civil claimant exists and has proper access
await this.validateCivilClaimant(civilClaimantId);
}apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/SendToPrisonAdmin/SendToPrisonAdmin.tsx (2)
5-5: LGTM: Import statement is correctly updated
The addition of InputFileUpload to the imports is appropriate for the new file upload functionality.
40-43: LGTM: Proper integration of defendantId with useS3Upload
The hook usage correctly incorporates the defendantId parameter, aligning with the PR's objective to connect case files to defendants.
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (2)
40-44: LGTM! Imports are well-structured
The imports follow NestJS conventions and are properly scoped to include only the necessary civil claimant-related entities.
116-121: LGTM! Comprehensive security measures implemented
The method is well-protected with appropriate guards:
- CaseTypeGuard ensures only indictment cases are processed
- CivilClaimantExistsGuard validates the civil claimant
- CaseWriteGuard and LimitedAccessWriteCaseFileGuard ensure proper access control
apps/judicial-system/api/src/app/modules/backend/backend.service.ts (1)
269-281: Implementation approach looks good
The implementation correctly handles the connection of case files to defendants or civil claimants through URL path segments. The limited access version appropriately restricts functionality to civil claimants only.
Let's verify the API endpoints are correctly implemented in the backend:
Also applies to: 449-457
✅ Verification successful
Let me verify the API endpoints by checking the backend implementation.
Implementation correctly handles case file connections
The backend implementation properly supports the connection of case files to both defendants and civil claimants through URL path segments. The code shows:
- Regular case file endpoints at
case/:id/filewith defendant/civil claimant connections via/defendant/:idor/civilClaimant/:idpath segments - Limited access endpoints at
case/:id/limitedAccess/filewith civil claimant connections via/civilClaimant/:idpath segment - Proper handling of file operations (create, get, delete) for both regular and limited access scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the API endpoints for connecting case files
# Check for the existence of corresponding API route handlers
rg -A 5 "POST.*case/.*file" --glob '!*.{ts,js}'
# Check for any existing usage of these endpoints
rg -A 5 "createCaseFile.*defendant|civilClaimant" --glob '*.{ts,tsx}'
Length of output: 103531
Script:
#!/bin/bash
# Check for the backend API route handlers for case files
ast-grep --pattern $'@(Get|Post|Put|Delete|Patch)($$$)
class $_ {
$$$
createCaseFile($$$) {
$$$
}
$$$
}'
# Check for any route definitions in the backend
rg -A 5 "case/.*file" --glob '*.{ts,js}'
Length of output: 5583
apps/judicial-system/web/src/utils/hooks/useS3Upload/useS3Upload.ts (4)
192-196: LGTM! Hook signature follows TypeScript best practices.
The addition of optional parameters is well-typed and follows naming conventions.
278-285: LGTM! Dependencies array is properly updated.
The dependencies array correctly includes the new parameters, following React hooks best practices.
262-263: Consider adding validation for mutual exclusivity.
Since a case file should likely be associated with either a defendant or a civil claimant (but not both), consider adding validation to ensure mutual exclusivity.
Let's verify if there are any existing validations or similar patterns in the codebase:
192-196: Consider adding access control validation.
While the hook uses limitedAccess for basic authorization, consider adding validation to ensure the user has permission to associate files with the specified defendant or civil claimant.
Let's check for existing access control patterns:
Also applies to: 262-263
✅ Verification successful
Let me search for more specific access control patterns in the judicial system application:
Let me search for more specific access control patterns in the judicial system app:
Based on the findings, I can now provide a final response:
Access control is properly implemented through multiple layers
The codebase shows a robust access control system:
- Backend guards enforce access control at the API level (CaseWriteGuard, CaseReadGuard, LimitedAccessCaseExistsGuard)
- Frontend uses
limitedAccesscontext to control file operations and UI rendering - File operations are protected by specific guards (WriteCaseFileGuard, ViewCaseFileGuard)
- Role-based access control is implemented through RolesGuard and role-specific rules
The current implementation in useS3Upload already leverages these security controls by:
- Using
limitedAccesscontext to determine appropriate API endpoints - Inheriting access control from backend guards
- Operating within the user's authorized scope
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for permission checks related to defendants and claimants
rg -B 5 "hasPermission|canAccess|isAuthorized"
# Look for guard implementations
ast-grep --pattern 'class $_Guard implements CanActivate {
$$$
canActivate($_) {
$$$
}
}'
Length of output: 29005
Script:
#!/bin/bash
# Search for access control related code in the judicial system app
rg -B 5 "limitedAccess|useCase|permission|auth" apps/judicial-system/
# Look for auth guards in the judicial system app
ast-grep --pattern 'class $_Guard {
$$$
canActivate($_) {
$$$
}
}'
# Search for auth context usage
rg -B 5 "useAuth|AuthContext" apps/judicial-system/
Length of output: 109290
Script:
#!/bin/bash
# Search for access control related code in the judicial system backend
rg -B 5 "canActivate|@UseGuards|@Authorized" apps/judicial-system/backend/
# Look for auth middleware or guards
ast-grep --pattern 'class $_Guard {
$$$
canActivate($_) {
$$$
}
}'
# Search for auth context and hooks in the web app
rg -B 5 "useAuth|AuthContext|limitedAccess" apps/judicial-system/web/src/components/
Length of output: 100655
apps/judicial-system/backend/src/app/modules/file/file.service.ts (1)
34-34: LGTM: Import statement is correctly placed and necessary
The import of CivilClaimant and Defendant entities is well-organized and aligns with the new functionality.
apps/judicial-system/backend/src/app/modules/file/guards/writeCaseFile.guard.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/file/guards/writeCaseFile.guard.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/file/file.controller.ts
Outdated
Show resolved
Hide resolved
...udicial-system/backend/src/app/modules/file/test/fileController/createCaseFileGuards.spec.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/file/dto/createFile.input.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/backend/backend.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/backend/backend.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/backend/backend.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/file/file.service.ts
Outdated
Show resolved
Hide resolved
…er-case-files-connection
apps/judicial-system/backend/src/app/modules/file/file.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/file/file.controller.ts (2)
137-157: Consider expanding role accessThe endpoint is currently restricted to only
publicProsecutorStaffRule. Consider allowing other relevant roles (like prosecutors and prosecutor representatives) who might need to create defendant-related files.Implementation looks good
The method is well-implemented with proper guards, parameter validation, logging, and service delegation.
159-190: Reconsider empty role rules implementationThe endpoint is currently inaccessible due to empty
@RolesRules(). Consider:
- Either removing the endpoint if it's not needed yet
- Or implementing it with proper role access if it's needed soon
- Or adding a TODO comment with timeline for role implementation
Having an inaccessible endpoint in production could lead to confusion.
Implementation looks good
The method is well-implemented with proper guards, parameter validation, logging, and service delegation. The undefined handling for the defendant parameter is correct.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/file/file.controller.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/file/file.controller.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/file/file.controller.ts (1)
52-59: LGTM: Imports are well-organized
The new imports are properly structured and necessary for the added functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apps/judicial-system/backend/src/app/modules/file/guards/limitedAccessWriteCaseFile.guard.ts (1)
The guard needs to be enhanced to validate defender-defendant associations
The current implementation of
LimitedAccessWriteCaseFileGuardonly checks the user's role and file category but does not validate if the defender is actually associated with the defendant for whom they are trying to access/modify case files. Based on the codebase:
- The Case model has
defendantsandcivilClaimantsassociations- The write guard only checks
UserRole.DEFENDERand file categories- No validation exists to ensure the defender can only access their assigned defendants' files
Required changes:
- Add validation in the guard to check if the defender's nationalId matches the defenderNationalId of any defendant in the case
- For civil cases, verify the defender's association with relevant civil claimants
🔗 Analysis chain
Line range hint
1-67: Consider enhancing the guard for the new defendant/claimant associations.Given that this PR introduces connections between case files and defendants/civil claimants, consider extending this guard to validate these new associations. For example:
- Verify that a defender can only access case files for their assigned defendants
- Ensure civil claimants can only access their own case files
Let's verify the current access patterns in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other guards or middleware that might handle defendant/claimant access rg -t typescript "implements CanActivate" apps/judicial-system/backend/src/ # Search for defender access patterns ast-grep --pattern 'if ($user.role === UserRole.DEFENDER) { $$$ }'Length of output: 181
Script:
#!/bin/bash # Search for guard implementations rg "implements CanActivate" -t ts apps/judicial-system/backend/src/ # Search for defender role checks rg "UserRole.DEFENDER" -A 5 -B 2 apps/judicial-system/backend/src/ # Search for case file access patterns rg "CaseFileCategory" -A 5 -B 2 apps/judicial-system/backend/src/ # Look for defendant/claimant associations rg "defendant|claimant" -t ts apps/judicial-system/backend/src/Length of output: 111646
Script:
#!/bin/bash # Search for defender/defendant access patterns in view guard rg -A 10 -B 2 "defenderNationalId|defendantId|civilClaimantId" apps/judicial-system/backend/src/app/modules/file/guards/limitedAccessViewCaseFile.guard.ts # Search for defender/defendant access patterns in write guard rg -A 10 -B 2 "defenderNationalId|defendantId|civilClaimantId" apps/judicial-system/backend/src/app/modules/file/guards/limitedAccessWriteCaseFile.guard.ts # Look for defender/defendant/claimant associations in case model rg -A 5 -B 2 "defendants|civilClaimants" apps/judicial-system/backend/src/app/modules/case/models/case.model.tsLength of output: 901
Script:
#!/bin/bash # Look for defender access validation in the view guard rg -A 15 "if \(user\.role === UserRole\.DEFENDER\)" apps/judicial-system/backend/src/app/modules/file/guards/limitedAccessViewCaseFile.guard.ts # Look for defender access validation in the write guard rg -A 15 "if \(user\.role === UserRole\.DEFENDER\)" apps/judicial-system/backend/src/app/modules/file/guards/limitedAccessWriteCaseFile.guard.ts # Look for defender-defendant associations in the case service rg -A 10 "defenderNationalId|defendantId" apps/judicial-system/backend/src/app/modules/case/case.service.tsLength of output: 1823
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (2)
40-44: Consider restructuring the module organization.Civil claimant related entities are currently imported from the 'defendant' module, which seems counterintuitive. Consider creating a separate 'civil-claimant' module to better represent the domain model and improve code organization.
116-121: Consider optimizing guard order.For better performance, consider reordering the guards to check role-based access before checking entity existence:
CaseTypeGuard(keeps current position as it's type-based)CaseWriteGuard(move up as it's permission-based)CivilClaimantExistsGuard(move down as it requires DB access)LimitedAccessWriteCaseFileGuard(keeps current position)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/file/guards/limitedAccessWriteCaseFile.guard.ts(1 hunks)apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/file/guards/limitedAccessWriteCaseFile.guard.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/file/guards/limitedAccessWriteCaseFile.guard.ts (1)
24-28: Improved error handling for user validation.
The reorganization of the user validation check improves the code by failing fast when the user is missing. This is a good practice as it prevents unnecessary processing and provides clear error messages.
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (1)
128-135: TypeScript implementation looks good!
The code follows TypeScript best practices with proper use of types, decorators, and parameter definitions.
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
apps/judicial-system/backend/src/app/modules/file/guards/createCivilClaimantCaseFile.guard.ts (1)
10-25: Add TypeScript improvements and documentationThe guard implementation could benefit from better type safety and documentation.
Consider applying these improvements:
+/** + * Guard to ensure that case files can only be created for civil claims. + * Throws ForbiddenException for non-civil claim categories. + */ @Injectable() export class CreateCivilClaimantCaseFileGuard implements CanActivate { + /** + * Validates that the case file category is CIVIL_CLAIM + * @param context - The execution context + * @returns true if the category is valid + * @throws ForbiddenException if the category is invalid + */ canActivate(context: ExecutionContext): boolean { const request = context.switchToHttp().getRequest() - - const caseFileCategory: CaseFileCategory = request.body?.category + + interface CreateCaseFileBody { + category: CaseFileCategory + } + + const body = request.body as CreateCaseFileBody + const caseFileCategory = body?.categoryapps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (1)
145-149: Validate parameters before spreadingThe spread operator usage
{ ...createFile, civilClaimantId }could potentially allow unexpected properties to pass through. Consider explicitly picking required properties:- { ...createFile, civilClaimantId }, + { + name: createFile.name, + size: createFile.size, + type: createFile.type, + civilClaimantId + },apps/judicial-system/backend/src/app/modules/case/case.service.ts (5)
Line range hint
91-173: Simplify optional properties in theUpdateCaseinterfaceIn the
UpdateCaseinterface, several properties are declared as optional using?and also include| nullin their type definitions, such ascourtCaseNumber?: string | null. In TypeScript, marking a property as optional (?) already allows it to beundefined. Including| nullcan be redundant unless there's a specific need to differentiate betweenundefinedandnull. Consider removing| nullfrom optional properties to simplify the type definitions.
Line range hint
759-778: Address potential SQL injection vulnerabilities in raw queriesThe code uses string interpolation within
Sequelize.literalto construct raw SQL queries that include user-provided data (defendant.nationalIdanddefendant.name):id: { [Op.in]: Sequelize.literal(` (SELECT case_id FROM defendant WHERE national_id = '${defendant.nationalId}' AND name = '${defendant.name}' )`), },This approach can lead to SQL injection vulnerabilities. It's important to parameterize queries to prevent malicious input from altering the SQL command. Consider using replacements provided by Sequelize to safely inject variables into raw queries.
Apply this diff to fix the issue:
id: { [Op.in]: Sequelize.literal(` (SELECT case_id FROM defendant - WHERE national_id = '${defendant.nationalId}' - AND name = '${defendant.name}' + WHERE national_id = :nationalId + AND name = :name )`), + }, +}, +{ + replacements: { + nationalId: defendant.nationalId, + name: defendant.name, + },This change uses named replacements to safely inject
nationalIdandnameinto the SQL query.
Line range hint
907-1015: Refactor complex conditional logic inaddMessagesForUpdatedCaseToQueueThe
addMessagesForUpdatedCaseToQueuemethod contains extensive nested conditional statements handling various case state transitions and scenarios. This complexity can make the code difficult to read, maintain, and test. Consider refactoring this method to improve clarity and maintainability by:
- Extracting conditional blocks into smaller, well-named helper methods.
- Utilizing a state machine pattern to manage state transitions and associated actions.
- Implementing strategy patterns to handle different case types and their unique behaviors.
Line range hint
1097-1106: Handle potential null values when accessing user propertiesIn the
requestRulingSignaturemethod, the code accessestheCase.judge?.mobileNumberandtheCase.judge?.namewithout checking if these properties are not null or undefined:const pdf = await getRulingPdfAsString(theCase, this.formatMessage); return this.signingService.requestSignature( theCase.judge?.mobileNumber ?? '', 'Undirrita skjal - Öryggistala', theCase.judge?.name ?? '', 'Ísland', 'ruling.pdf', pdf, );If
theCase.judgeis undefined, this could lead to unexpected behavior or signing requests with empty judge information. Ensure that the judge information is available before attempting to request a signature, and handle cases where it might be missing.Apply this diff to add a check:
+if (!theCase.judge || !theCase.judge.mobileNumber || !theCase.judge.name) { + throw new InternalServerErrorException('Judge information is missing for signature request'); +} const pdf = await getRulingPdfAsString(theCase, this.formatMessage); return this.signingService.requestSignature( theCase.judge.mobileNumber, 'Undirrita skjal - Öryggistala', theCase.judge.name, 'Ísland', 'ruling.pdf', pdf, );
Line range hint
679-725: Avoid reassigning theupdateparameter within theupdatemethodIn the
updatemethod, theupdateparameter is reassigned whenreceivingCaseis true:if (receivingCase) { update = transitionCase(CaseTransition.RECEIVE, theCase, user, update); }Reassigning function parameters can lead to unexpected side effects and make the code harder to understand and maintain. Consider assigning the result to a new variable or directly updating the properties without reassigning the parameter.
Apply this diff to fix the issue:
if (receivingCase) { - update = transitionCase(CaseTransition.RECEIVE, theCase, user, update); + const transitionedUpdate = transitionCase(CaseTransition.RECEIVE, theCase, user, update); + Object.assign(update, transitionedUpdate); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/judicial-system/backend/src/app/modules/case/case.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/file/file.controller.ts(2 hunks)apps/judicial-system/backend/src/app/modules/file/file.service.ts(2 hunks)apps/judicial-system/backend/src/app/modules/file/guards/createCivilClaimantCaseFile.guard.ts(1 hunks)apps/judicial-system/backend/src/app/modules/file/guards/createDefendantCaseFile.guard.ts(1 hunks)apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/backend/src/app/modules/file/file.service.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/file.controller.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/guards/createCivilClaimantCaseFile.guard.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/guards/createDefendantCaseFile.guard.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
📓 Learnings (1)
apps/judicial-system/backend/src/app/modules/file/guards/createDefendantCaseFile.guard.ts (1)
Learnt from: oddsson
PR: island-is/island.is#16138
File: apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts:74-87
Timestamp: 2024-11-12T15:15:26.274Z
Learning: User prefers to keep similar conditional statements separate rather than combining them into a single conditional with logical operators.
🔇 Additional comments (6)
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts (3)
40-44: LGTM: Imports are well-organized
The new imports for civil claimant functionality are properly grouped and follow TypeScript best practices.
128-128:
Security concern: Endpoint has no role restrictions.
The empty @RolesRules() decorator makes this endpoint accessible to all authenticated users, which is a security risk. Either implement proper role restrictions or document why this is intentionally unrestricted.
134-150: 🛠️ Refactor suggestion
Consider refactoring to reduce code duplication and improve parameter handling.
The method largely duplicates the logic from createCaseFile. Consider creating a base method that both endpoints can use with a parameter object for better maintainability.
apps/judicial-system/backend/src/app/modules/file/file.controller.ts (2)
132-161: Verify Access Permissions for createDefendantCaseFile Method
The createDefendantCaseFile method is currently restricted to users with the publicProsecutorStaffRule role. Please verify that this aligns with the intended access control requirements. If additional roles should have access, consider updating the @RolesRules decorator accordingly.
170-192:
Critical Security Issue: Unrestricted Access to createCivilClaimantCaseFile Endpoint
The createCivilClaimantCaseFile method currently lacks role restrictions due to an empty @RolesRules() decorator. This means that any authenticated user could potentially access this endpoint, which poses a significant security risk.
Apply this diff to restrict access to appropriate roles:
- @RolesRules() // This endpoint is not used by any role at the moment
+ @RolesRules(publicProsecutorStaffRule)Or specify the relevant roles that should have access to this endpoint to ensure proper authorization.
⛔ Skipped due to learnings
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-11-12T15:15:20.157Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
Line range hint 912-915: Ensure messages are sent when reopening cases
In the condition checking for state transitions to CaseState.RECEIVED, messages are only sent if the previous state was CaseState.SUBMITTED:
if (
updatedCase.state === CaseState.RECEIVED &&
theCase.state === CaseState.SUBMITTED
) {
// ...
}If a case is reopened from a state other than SUBMITTED, notification messages may not be sent as expected. Verify whether this logic aligns with business requirements. If reopening cases should also trigger notifications, consider adjusting the condition to accommodate that.
apps/judicial-system/backend/src/app/modules/file/guards/createCivilClaimantCaseFile.guard.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/file/guards/createCivilClaimantCaseFile.guard.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/file/guards/createDefendantCaseFile.guard.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/file/guards/createDefendantCaseFile.guard.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
apps/judicial-system/api/src/app/modules/file/dto/createCivilClaimantFile.input.ts (2)
7-12: Add JSDoc documentation for better code maintainability.Consider adding documentation to describe the purpose of this input type and its field.
+/** + * Input type for creating a case file associated with a civil claimant. + */ @InputType() export class CreateCivilClaimantFileInput extends CreateFileInput { + /** + * The unique identifier of the civil claimant to associate with this file. + */ @Allow() @Field(() => ID) readonly civilClaimantId!: string }
9-11: Consider adding validation rules for civilClaimantId.The field currently only has
@Allow()validation. Consider adding more specific validation rules to ensure data integrity.@Allow() + @IsString() + @IsNotEmpty() + @Matches(/^[a-zA-Z0-9-]+$/, { + message: 'Civil claimant ID must contain only alphanumeric characters and hyphens', + }) @Field(() => ID) readonly civilClaimantId!: stringapps/judicial-system/api/src/app/modules/file/file.resolver.ts (3)
98-98: Consider adding a specific audit action for defendant filesCurrently using generic
CREATE_FILEaction. Consider adding a more specific audit action likeCREATE_DEFENDANT_FILEto improve audit trail clarity.
120-120: Consider adding a specific audit action for civil claimant filesCurrently using generic
CREATE_FILEaction. Consider adding a more specific audit action likeCREATE_CIVIL_CLAIMANT_FILEto improve audit trail clarity.
121-125: Maintain consistent formatting for backend service callsThe backend service call is split across multiple lines while other similar calls in the file are on a single line. Consider maintaining consistent formatting:
- backendService.createCivilClaimantCaseFile( - caseId, - createFile, - civilClaimantId, - ), + backendService.createCivilClaimantCaseFile(caseId, createFile, civilClaimantId),apps/judicial-system/api/src/app/modules/backend/backend.service.ts (1)
281-290: Consider reducing code duplicationThe
createCivilClaimantCaseFileandlimitedAccessCreateCivilClaimantCaseFilemethods share similar logic. Consider extracting the common functionality into a private method.+ private createCivilClaimantFile( + id: string, + createFile: CreateCaseFileInput, + civilClaimantId: string, + limitedAccess: boolean = false + ): Promise<CaseFile> { + if (!civilClaimantId) { + throw new Error('civilClaimantId is required'); + } + const accessPath = limitedAccess ? '/limitedAccess' : ''; + return this.post( + `case/${id}${accessPath}/civilClaimant/${civilClaimantId}/file`, + createFile, + ); + } createCivilClaimantCaseFile( id: string, createFile: CreateCaseFileInput, civilClaimantId?: string, ): Promise<CaseFile> { - if (!civilClaimantId) { - throw new Error('civilClaimantId is required'); - } - return this.post( - `case/${id}/civilClaimant/${civilClaimantId}/file`, - createFile, - ) + return this.createCivilClaimantFile(id, createFile, civilClaimantId); } limitedAccessCreateCivilClaimantCaseFile( id: string, createFile: CreateCaseFileInput, civilClaimantId?: string, ): Promise<CaseFile> { - if (!civilClaimantId) { - throw new Error('civilClaimantId is required'); - } - return this.post( - `case/${id}/limitedAccess/civilClaimant/${civilClaimantId}/file`, - createFile, - ) + return this.createCivilClaimantFile(id, createFile, civilClaimantId, true); }Also applies to: 461-470
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/judicial-system/api/src/app/modules/backend/backend.service.ts(2 hunks)apps/judicial-system/api/src/app/modules/file/dto/createCivilClaimantFile.input.ts(1 hunks)apps/judicial-system/api/src/app/modules/file/dto/createDefendantFile.input.ts(1 hunks)apps/judicial-system/api/src/app/modules/file/file.resolver.ts(2 hunks)apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/judicial-system/api/src/app/modules/backend/backend.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/src/app/modules/file/dto/createCivilClaimantFile.input.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/src/app/modules/file/dto/createDefendantFile.input.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/src/app/modules/file/file.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (10)
apps/judicial-system/api/src/app/modules/file/dto/createDefendantFile.input.ts (1)
5-8: Verify the base class implementation
The class correctly extends CreateFileInput. Let's verify the base class implementation to ensure all required fields are properly inherited.
✅ Verification successful
Base class implementation verified and properly inherited
The CreateDefendantFileInput correctly extends CreateFileInput from the judicial system API, which provides all necessary base fields including:
caseId(ID type)type(String type)- Various other optional fields with proper decorators
The derived class adds its specific field:
defendantId(ID type)
All decorators and type definitions are properly set up in both base and derived classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the base class implementation and its usage
# Find the base class definition
echo "Base class definition:"
ast-grep --pattern 'export class CreateFileInput {
$$$
}'
# Find other implementations extending the same base class
echo "\nOther implementations:"
ast-grep --pattern 'class $_ extends CreateFileInput {
$$$
}'
Length of output: 1341
Script:
#!/bin/bash
# Let's check the actual implementation of CreateFileInput class
echo "Searching for CreateFileInput implementation:"
rg -A 10 "export class CreateFileInput"
# Also check for any Field decorators in CreateFileInput
echo "\nSearching for Field decorators in CreateFileInput:"
rg "@Field" apps/judicial-system/api/src/app/modules/file/dto/createFile.input.ts
Length of output: 2648
apps/judicial-system/api/src/app/modules/file/dto/createCivilClaimantFile.input.ts (1)
1-12: LGTM! The implementation follows NestJS and GraphQL best practices.
The class is well-structured, properly extends the base input type, and correctly implements the GraphQL field definitions.
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts (3)
18-18: LGTM!
The import follows TypeScript best practices and maintains consistent naming conventions.
80-102: Add validation and improve error handling for civilClaimantId
While the implementation correctly handles the new parameter, consider adding validation and improving error handling:
- Validate the
civilClaimantIdformat before passing it to the backend service - Update the debug logging to include the civil claimant context
- Consider enhancing the audit trail with civil claimant information
80-102: Consider implementing a validation pipe for input sanitization
To improve the robustness of the API, consider implementing a custom validation pipe that:
- Validates the format and existence of civilClaimantId
- Ensures the civil claimant has proper access rights to the case
- Sanitizes input to prevent potential security issues
This aligns with NextJS best practices for API route handling.
Let's check if similar validation patterns exist in the codebase:
apps/judicial-system/api/src/app/modules/file/file.resolver.ts (3)
18-19: LGTM!
The new imports are properly organized and follow TypeScript best practices.
82-102: Implementation aligns with best practices and previous feedback
The implementation follows the established pattern and properly handles:
- GraphQL mutation decoration
- Input type validation
- Logging
- Audit trail
- Separation of concerns
This also aligns with the previous review feedback about using separate endpoints.
84-85: Verify the implementation of new input types
Let's verify that the new input types are properly implemented with all necessary fields.
Also applies to: 106-107
✅ Verification successful
Let me try a different approach to find these input types, as they might be defined in a different file or with different patterns.
Let me check the base CreateFileInput class since both input types extend from it.
Input types are properly implemented
The verification shows that both input types are correctly implemented:
-
CreateDefendantFileInputextendsCreateFileInputand adds:defendantId: string(ID field)
-
CreateCivilClaimantFileInputextendsCreateFileInputand adds:civilClaimantId: string(ID field)
Both inherit from the base CreateFileInput which provides:
caseId: string(ID field)type: stringcategory?: CaseFileCategory(optional)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of CreateDefendantFileInput and CreateCivilClaimantFileInput
# Search for the input type definitions
echo "Searching for CreateDefendantFileInput definition:"
ast-grep --pattern 'class CreateDefendantFileInput {
$$$
}'
echo "Searching for CreateCivilClaimantFileInput definition:"
ast-grep --pattern 'class CreateCivilClaimantFileInput {
$$$
}'
Length of output: 361
Script:
#!/bin/bash
# Search for files containing CreateDefendantFileInput
echo "Files containing CreateDefendantFileInput:"
rg "CreateDefendantFileInput" -l
# Show the content of files containing CreateDefendantFileInput
echo -e "\nContent of files with CreateDefendantFileInput:"
rg "CreateDefendantFileInput" -A 10 -B 2
# Search for files containing CreateCivilClaimantFileInput
echo -e "\nFiles containing CreateCivilClaimantFileInput:"
rg "CreateCivilClaimantFileInput" -l
# Show the content of files containing CreateCivilClaimantFileInput
echo -e "\nContent of files with CreateCivilClaimantFileInput:"
rg "CreateCivilClaimantFileInput" -A 10 -B 2
Length of output: 11506
Script:
#!/bin/bash
# Search for CreateFileInput definition
echo "Content of CreateFileInput:"
rg "class CreateFileInput" -A 15 -B 2
Length of output: 3997
apps/judicial-system/api/src/app/modules/backend/backend.service.ts (2)
273-279:
Fix URL construction and improve type safety
There are several issues in this method:
- The URL construction is incorrect - missing a forward slash between 'defendant' and defendantId
- The createFile parameter lacks type safety
Apply this fix:
createDefendantCaseFile(
id: string,
- createFile: unknown,
+ createFile: CreateCaseFileInput,
defendantId: string,
): Promise<CaseFile> {
- return this.post(`case/${id}/defendant${defendantId}/file`, createFile)
+ return this.post(`case/${id}/defendant/${defendantId}/file`, createFile)
}461-470:
Fix URL path and add null check
The method has several issues:
- Typo in URL path ('$' after 'limitedAccess')
- No handling for undefined civilClaimantId
- The createFile parameter lacks type safety
Apply these fixes:
limitedAccessCreateCivilClaimantCaseFile(
id: string,
- createFile: unknown,
+ createFile: CreateCaseFileInput,
civilClaimantId?: string,
): Promise<CaseFile> {
+ if (!civilClaimantId) {
+ throw new Error('civilClaimantId is required');
+ }
return this.post(
- `case/${id}/limitedAccess$/civilClaimant/${civilClaimantId}/file`,
+ `case/${id}/limitedAccess/civilClaimant/${civilClaimantId}/file`,
createFile,
)
}Likely invalid or redundant comment.
apps/judicial-system/api/src/app/modules/file/dto/createDefendantFile.input.ts
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/backend/backend.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/file/limitedAccessFile.controller.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
apps/judicial-system/web/src/utils/hooks/useS3Upload/createDefendantFile.graphql (1)
2-4: Consider returning additional useful fields.Currently, the mutation only returns the
idfield. Consider returning additional fields that might be immediately needed by the client to avoid subsequent queries. Common fields might includefileName,fileSize, oruploadedAt.Example enhancement:
mutation CreateDefendantFile($input: CreateDefendantFileInput!) { createDefendantFile(input: $input) { id + fileName + fileSize + uploadedAt + defendantId } }apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateCivilClaimantFile.graphql (1)
1-7: Consider expanding the return fields for better client-side efficiency.While the mutation structure is well-defined, returning only the
idmight require additional queries if the client needs more information about the created file. Consider including commonly needed fields in the response to reduce the number of round-trips.Example enhancement:
mutation LimitedAccessCreateCivilClaimantFile( $input: CreateCivilClaimantFileInput! ) { limitedAccessCreateCivilClaimantFile(input: $input) { id + name + size + type + status } }apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateDefendantFile.graphql (1)
1-5: Consider enhancing the mutation response type.While the current implementation is correct, consider adding fields like
successanderrorsto handle potential failure cases more gracefully.mutation LimitedAccessCreateDefendantFile($input: CreateDefendantFileInput!) { limitedAccessCreateDefendantFile(input: $input) { id + success + errors { + message + code + } } }apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts (1)
81-105: Enhance error handling and logging for defendant file creation.While the implementation is solid, consider these improvements:
- Add validation for
defendantIdformat- Enhance logging with additional context (e.g., file type, size)
- Consider structured logging for better observability
limitedAccessCreateDefendantFile( @Args('input', { type: () => CreateDefendantFileInput }) input: CreateDefendantFileInput, @CurrentGraphQlUser() user: User, @Context('dataSources') { backendService }: { backendService: BackendService }, ): Promise<CaseFile> { const { caseId, defendantId, ...createFile } = input + if (!defendantId?.trim()) { + throw new BadRequestException('Invalid defendantId provided'); + } this.logger.debug( - `Creating a file for case ${caseId} and defendant ${defendantId}`, + { + message: `Creating a file for case ${caseId} and defendant ${defendantId}`, + context: { + caseId, + defendantId, + fileType: createFile.type, + size: createFile.size + } + } ) return this.auditTrailService.audit( user.id, AuditedAction.CREATE_FILE, backendService.limitedAccessCreateDefendantCaseFile( caseId, createFile, defendantId, ), - (file) => file.id, + (file) => ({ + fileId: file.id, + context: { defendantId } + }), ) }apps/judicial-system/web/src/utils/hooks/useS3Upload/useS3Upload.ts (2)
209-213: Add TypeScript interface foruseS3UploadparametersTo enhance type safety and readability, consider defining a TypeScript interface for the parameters of the
useS3Uploadhook, especially since it now accepts optional parameters.+interface UseS3UploadParams { + caseId: string + defendantId?: string + civilClaimantId?: string +} -const useS3Upload = ( - caseId: string, - defendantId?: string, - civilClaimantId?: string, -) => { +const useS3Upload = ({ + caseId, + defendantId, + civilClaimantId, +}: UseS3UploadParams) => {
278-304: Provide specific error messages for better debuggingUpdating the error messages to reflect the specific context will aid in troubleshooting.
if (!createdFile?.id) { - throw Error('Failed to add file to case') + throw Error('Failed to add file to case for defendant') }Apply similar changes to the other functions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/judicial-system/api/src/app/modules/backend/backend.service.ts(2 hunks)apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts(2 hunks)apps/judicial-system/web/src/utils/hooks/useS3Upload/createCivilClaimantFile.graphql(1 hunks)apps/judicial-system/web/src/utils/hooks/useS3Upload/createDefendantFile.graphql(1 hunks)apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateCivilClaimantFile.graphql(1 hunks)apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateDefendantFile.graphql(1 hunks)apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateFile.graphql(0 hunks)apps/judicial-system/web/src/utils/hooks/useS3Upload/useS3Upload.ts(5 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateFile.graphql
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/api/src/app/modules/backend/backend.service.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/judicial-system/web/src/utils/hooks/useS3Upload/createDefendantFile.graphql (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateDefendantFile.graphql (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/utils/hooks/useS3Upload/createCivilClaimantFile.graphql (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateCivilClaimantFile.graphql (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/utils/hooks/useS3Upload/useS3Upload.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (8)
apps/judicial-system/web/src/utils/hooks/useS3Upload/createDefendantFile.graphql (1)
1-5: LGTM! The mutation structure follows GraphQL best practices.
The mutation is well-structured with a clear purpose and follows GraphQL naming conventions.
apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateCivilClaimantFile.graphql (1)
1-4: Verify access control implementation for limited access mutation.
The mutation name suggests restricted access. Let's verify the implementation of access controls in the resolver.
✅ Verification successful
Let me gather more information about the access control implementation.
Let me check for the JWT guard implementation and any additional security measures.
Access control is properly implemented through JWT authentication
The resolver is secured with the following measures:
@UseGuards(JwtGraphQlAuthGuard)decorator enforces JWT authentication@CurrentGraphQlUser()decorator injects the authenticated user- The resolver is properly scoped in a dedicated module with limited access endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the resolver implementation to verify access control checks
# Look for the resolver implementation
ast-grep --pattern 'class $_ {
$$$
limitedAccessCreateCivilClaimantFile($_, $_) {
$$$
}
$$$
}'
# Search for any auth-related decorators or checks
rg -B 3 -A 10 "limitedAccessCreateCivilClaimantFile"
Length of output: 9210
Script:
#!/bin/bash
# Search for auth-related decorators and guards in the resolver file
rg -B 5 -A 1 "@(UseGuards|Authorized|Auth)" apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts
# Search for the resolver class definition and its decorators
ast-grep --pattern '@$_($$$)
class LimitedAccessFileResolver {
$$$
}'
# Look for any auth-related imports
rg -B 1 -A 3 "import.*@nestjs/(security|auth|authorization)" apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts
Length of output: 606
Script:
#!/bin/bash
# Look for the JWT guard implementation
rg -B 3 -A 10 "class JwtGraphQlAuthGuard"
# Check for any additional security middleware or decorators in the module
rg -B 3 -A 10 "LimitedAccessFileResolver" --type ts
# Look for the CurrentGraphQlUser decorator implementation
ast-grep --pattern 'function CurrentGraphQlUser() {
$$$
}'
Length of output: 3939
apps/judicial-system/web/src/utils/hooks/useS3Upload/limitedAccessCreateDefendantFile.graphql (1)
1-2: Verify access control implementation for limited access.
The mutation name implies restricted access. Let's verify the corresponding resolver implements proper access control checks.
✅ Verification successful
Let me search for the resolver implementation to verify the access control.
Let me check if there are any access control decorators or guards specifically for this resolver.
Let me check what the JwtGraphQlAuthGuard does to understand the access control implementation.
Let me check the JWT auth guard implementation to understand the access control mechanism.
Access control is properly implemented through JWT authentication
The limitedAccessCreateDefendantFile mutation is protected by the JwtGraphQlAuthGuard which:
- Enforces JWT token validation using RS256 algorithm
- Validates token audience and issuer
- Extracts JWT from cookie or authorization header
- Requires authenticated users by default
The "limited access" in the mutation name refers to the scope of the operation (creating defendant files) rather than suggesting missing access controls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the resolver implementation and access control checks
# Look for the resolver implementation
echo "Searching for resolver implementation..."
ast-grep --pattern 'class $_ implements $_ {
$$$
async limitedAccessCreateDefendantFile($_, $_) {
$$$
}
$$$
}'
# Look for access control decorators or checks
echo "Searching for access control mechanisms..."
rg -A 5 "@UseGuards|@Authorized|checkAccess" --type ts
Length of output: 66222
Script:
#!/bin/bash
# Search for the limitedAccessCreateDefendantFile resolver implementation
ast-grep --pattern 'async limitedAccessCreateDefendantFile($_, $_) {
$$$
}'
# Search for any files containing limitedAccessCreateDefendantFile
rg -A 10 "limitedAccessCreateDefendantFile"
Length of output: 7269
Script:
#!/bin/bash
# Search for the class definition and decorators of LimitedAccessFileResolver
rg -B 5 "class LimitedAccessFileResolver" --type ts
# Search for any auth guards or decorators in the file
rg -B 5 "@UseGuards|@Authorized|@Scopes" "apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts"
Length of output: 1088
Script:
#!/bin/bash
# Search for JwtGraphQlAuthGuard implementation
rg -A 10 "class JwtGraphQlAuthGuard" --type ts
# Search for any related auth guards or middleware
rg -A 10 "JwtGraphQlAuthGuard" --type ts --glob '!*.spec.ts' --glob '!*.test.ts'
Length of output: 34021
Script:
#!/bin/bash
# Search for the AuthGuard implementation and related auth strategy
rg -A 10 "class AuthGuard" --type ts
# Search for JWT strategy implementation
rg -A 10 "class JwtStrategy" --type ts
Length of output: 4025
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.resolver.ts (3)
18-19: LGTM! Import statements are well-structured.
The new imports follow the existing pattern and naming conventions.
107-131: Similar improvements needed for civil claimant file creation.
The implementation needs the same enhancements as suggested for the defendant file creation.
limitedAccessCreateCivilClaimantFile(
@Args('input', { type: () => CreateCivilClaimantFileInput })
input: CreateCivilClaimantFileInput,
@CurrentGraphQlUser() user: User,
@Context('dataSources')
{ backendService }: { backendService: BackendService },
): Promise<CaseFile> {
const { caseId, civilClaimantId, ...createFile } = input
+ if (!civilClaimantId?.trim()) {
+ throw new BadRequestException('Invalid civilClaimantId provided');
+ }
this.logger.debug(
- `Creating a file for case ${caseId} and civil claimant ${civilClaimantId}`,
+ {
+ message: `Creating a file for case ${caseId} and civil claimant ${civilClaimantId}`,
+ context: {
+ caseId,
+ civilClaimantId,
+ fileType: createFile.type,
+ size: createFile.size
+ }
+ }
)
return this.auditTrailService.audit(
user.id,
AuditedAction.CREATE_FILE,
backendService.limitedAccessCreateCivilClaimantCaseFile(
caseId,
createFile,
civilClaimantId,
),
- (file) => file.id,
+ (file) => ({
+ fileId: file.id,
+ context: { civilClaimantId }
+ }),
)
}81-131: Verify related GraphQL and backend implementations.
Let's ensure all required components are in place for the new mutations.
✅ Verification successful
Let me try a different approach to verify these components.
All required implementations are in place
The verification shows that all necessary components are properly implemented:
- GraphQL mutations are defined in both resolver and schema files
- Backend service methods
limitedAccessCreateDefendantCaseFileandlimitedAccessCreateCivilClaimantCaseFileexist - Input types
CreateDefendantFileInputandCreateCivilClaimantFileInputare properly defined and used - Frontend hooks and GraphQL operations are implemented in the web application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GraphQL schema and related implementations
# Check if GraphQL schema includes the new mutations
echo "Checking GraphQL schema..."
rg -A 2 "type Mutation {" | rg -A 10 "limitedAccessCreate(Civil|Defendant)File"
# Verify backend service implementation
echo "Checking backend service..."
ast-grep --pattern 'class BackendService {
$$$
limitedAccessCreateDefendantCaseFile($$$) {
$$$
}
limitedAccessCreateCivilClaimantCaseFile($$$) {
$$$
}
$$$
}'
# Check input type definitions
echo "Checking input type definitions..."
fd -e ts "Create(Defendant|CivilClaimant)FileInput.ts$"
Length of output: 620
Script:
#!/bin/bash
# Check GraphQL schema with simpler patterns
echo "Checking GraphQL schema mutations..."
rg "limitedAccessCreateDefendantFile" -A 2
rg "limitedAccessCreateCivilClaimantFile" -A 2
# Check backend service with simpler pattern
echo -e "\nChecking backend service implementation..."
rg "class BackendService" -A 10
rg "limitedAccessCreateDefendantCaseFile" -A 2
rg "limitedAccessCreateCivilClaimantCaseFile" -A 2
# Check input types
echo -e "\nChecking input type definitions..."
rg "CreateDefendantFileInput" -A 2
rg "CreateCivilClaimantFileInput" -A 2
Length of output: 13245
apps/judicial-system/web/src/utils/hooks/useS3Upload/useS3Upload.ts (1)
357-368: Ensure all dependencies are included in useCallback dependency array
Double-check that all variables used within addFileToCaseState are included in the dependency array to prevent stale closures.
apps/judicial-system/web/src/utils/hooks/useS3Upload/createCivilClaimantFile.graphql (1)
1-5: Mutation CreateCivilClaimantFile is correctly defined
The GraphQL mutation is well-structured and aligns with the schema expectations.
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests