chore(j-s): Security&Integrity #16863
Conversation
WalkthroughThe changes in this pull request primarily focus on the integration and management of a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Datadog ReportAll test runs ✅ 21 Total Test Services: 0 Failed, 20 Passed Test ServicesThis report shows up to 10 services
|
gudjong
dismissed
coderabbitai[bot]’s stale review
All requests have been handled
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.controller.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.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/police/models/subpoenaInfo.model.ts (2)
5-13: Consider reordering type definitions to avoid potential circular dependencies.The
SubpoenaUpdateinterface extends fromSubpoenaInfobefore the class is defined. While TypeScript's declaration hoisting might handle this, it's better to define types in a clear dependency order.Consider moving the interface after the class definition or extracting the shared types to a separate interface.
15-21: Consider deriving keys dynamically to prevent maintenance issues.Maintaining a separate array of keys could lead to inconsistencies if the
SubpoenaUpdateinterface changes. Consider using TypeScript's type system to derive the keys automatically.-const subpoenaUpdateKeys: Array<keyof SubpoenaUpdate> = [ - 'serviceStatus', - 'comment', - 'servedBy', - 'defenderNationalId', - 'serviceDate', -] +const subpoenaUpdateKeys: Array<keyof SubpoenaUpdate> = Object.keys( + SubpoenaInfo.prototype +).filter((key) => key !== 'isNewValueSetAndDifferent' && key !== 'isSubpoenaInfoChanged') as Array<keyof SubpoenaUpdate>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/police/models/subpoenaInfo.model.ts(1 hunks)apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/police/models/subpoenaInfo.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."
📓 Learnings (1)
apps/judicial-system/backend/src/app/modules/police/models/subpoenaInfo.model.ts (1)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-11-12T15:15:11.835Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
🔇 Additional comments (3)
apps/judicial-system/backend/src/app/modules/police/models/subpoenaInfo.model.ts (3)
1-3: LGTM! Dependencies are properly imported.
The imports are well-organized and appropriate for the model's requirements.
35-49: LGTM! Properties are well-documented with Swagger decorators.
The class properties are properly defined and documented. The optional nature of the properties aligns with the subpoena update requirements.
24-33: Consider handling complex object comparisons.
The isNewValueSetAndDifferent method might not handle deep equality checks for objects or arrays correctly. This could lead to false positives when comparing complex values.
Consider using a deep equality comparison for object types:
private isNewValueSetAndDifferent = (
newValue: unknown,
oldValue: unknown,
): boolean => {
if (!newValue) return false;
if (typeof newValue === 'object' && newValue !== null) {
return JSON.stringify(newValue) !== JSON.stringify(oldValue);
}
return newValue !== oldValue;
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
apps/judicial-system/backend/src/app/modules/police/police.service.ts (2)
369-370: Address TODO comment for expired status handlingThe TODO comment indicates that expired status handling is missing. This could affect the completeness of the subpoena status tracking.
Would you like me to help implement the expired status handling or create a GitHub issue to track this task?
361-370: Simplify complex service status determinationThe nested ternary operations make the code harder to read and maintain.
Consider using a more readable approach:
- serviceStatus: response.deliveredToLawyer - ? ServiceStatus.DEFENDER - : response.prosecutedConfirmedSubpoenaThroughIslandis - ? ServiceStatus.ELECTRONICALLY - : response.deliveredOnPaper || response.delivered === true - ? ServiceStatus.IN_PERSON - : response.acknowledged === false && response.delivered === false - ? ServiceStatus.FAILED - : undefined, + serviceStatus: getServiceStatus(response),Add a helper function:
function getServiceStatus(response: SubpoenaResponse): ServiceStatus | undefined { if (response.deliveredToLawyer) { return ServiceStatus.DEFENDER; } if (response.prosecutedConfirmedSubpoenaThroughIslandis) { return ServiceStatus.ELECTRONICALLY; } if (response.deliveredOnPaper || response.delivered === true) { return ServiceStatus.IN_PERSON; } if (response.acknowledged === false && response.delivered === false) { return ServiceStatus.FAILED; } return undefined; }apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (3)
62-81: Consider enhancing type safety and readability of utility functions.The utility functions are well-structured, but could benefit from some improvements:
-const isNewValueSetAndDifferent = ( - newValue: unknown, - oldValue: unknown, -): boolean => Boolean(newValue) && newValue !== oldValue +const hasChanged = <T>(newValue: T | null | undefined, oldValue: T): boolean => + newValue != null && newValue !== oldValue -export const isSubpoenaInfoChanged = ( +export const hasSubpoenaInfoChanges = ( newSubpoenaInfo: SubpoenaInfo, oldSubpoenaInfo: SubpoenaInfo, -) => - subpoenaInfoKeys.some((key) => - isNewValueSetAndDifferent(newSubpoenaInfo[key], oldSubpoenaInfo[key]), - ) +): boolean => subpoenaInfoKeys.some((key) => + hasChanged(newSubpoenaInfo[key], oldSubpoenaInfo[key]) +)
219-219: Translate comment to English.The comment "Færa message handling í defendant service" should be in English for consistency.
-// Færa message handling í defendant service +// TODO: Move message handling to defendant service
198-236: Consider extracting defender choice reset logic.The defender choice update logic is complex and could benefit from being extracted into a separate method for better readability.
+private shouldResetDefenderChoice( + defendant: Defendant, + defenderChoice?: string, + defenderNationalId?: string, +): boolean { + return ( + defendant.isDefenderChoiceConfirmed && + ((defenderChoice && defendant.defenderChoice !== defenderChoice) || + (defenderNationalId && defendant.defenderNationalId !== defenderNationalId)) + ) +} if ( subpoena.case && subpoena.defendant && (defenderChoice || defenderNationalId || defenderName || defenderEmail || defenderPhoneNumber || requestedDefenderChoice || requestedDefenderNationalId || requestedDefenderName) ) { - const resetDefenderChoiceConfirmed = - subpoena.defendant?.isDefenderChoiceConfirmed && - ((defenderChoice && - subpoena.defendant?.defenderChoice !== defenderChoice) || - (defenderNationalId && - subpoena.defendant?.defenderNationalId !== defenderNationalId)) + const resetDefenderChoiceConfirmed = this.shouldResetDefenderChoice( + subpoena.defendant, + defenderChoice, + defenderNationalId + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/judicial-system/backend/src/app/modules/police/index.ts(1 hunks)apps/judicial-system/backend/src/app/modules/police/police.service.ts(5 hunks)apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/backend/src/app/modules/police/index.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/subpoena/subpoena.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/police/police.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."
📓 Learnings (3)
apps/judicial-system/backend/src/app/modules/police/index.ts (1)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-11-12T15:15:11.835Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (3)
Learnt from: gudjong
PR: island-is/island.is#16863
File: apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts:173-174
Timestamp: 2024-11-27T14:24:01.943Z
Learning: When updating records in the `SubpoenaService` class (`apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts`), prefer to log errors when multiple rows are affected instead of throwing an exception.
Learnt from: gudjong
PR: island-is/island.is#16863
File: apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts:163-216
Timestamp: 2024-11-27T14:25:48.464Z
Learning: When using Sequelize transactions in the backend services, exceptions thrown within the transaction callback are sufficient for rollback, and additional exception handling within the transaction is not necessary.
Learnt from: oddsson
PR: island-is/island.is#16329
File: apps/judicial-system/backend/src/app/modules/notification/subpoenaNotification.service.ts:0-0
Timestamp: 2024-11-12T15:15:26.274Z
Learning: In `apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts`, the method `findBySubpoenaId` throws an error if the subpoena is not found. Therefore, downstream code does not need to check if the result is undefined.
apps/judicial-system/backend/src/app/modules/police/police.service.ts (1)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-11-12T15:15:11.835Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
🔇 Additional comments (6)
apps/judicial-system/backend/src/app/modules/police/index.ts (1)
4-4: LGTM! Export of SubpoenaInfo interface
The addition of the SubpoenaInfo export aligns with the PR's objective to enhance the subpoena handling process.
apps/judicial-system/backend/src/app/modules/police/police.service.ts (2)
61-67: LGTM! Well-structured SubpoenaInfo interface
The interface properly encapsulates all necessary fields for subpoena status information with appropriate optionality.
Line range hint 395-402: LGTM! Robust error handling
The error handling follows the service's pattern with proper logging and clear error messages. The comment explains the behavior when the service is unavailable.
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (3)
86-86: LGTM: Sequelize injection properly implemented.
The Sequelize connection is correctly injected using the @InjectConnection() decorator.
258-280: LGTM: Finder methods properly implemented with improved error handling.
The finder methods are well-implemented with:
- Clear error messages
- Proper use of NotFoundException
- Consistent include patterns
359-383: LGTM with verification: Comprehensive subpoena status handling.
The implementation properly handles subpoena status updates with appropriate checks and authorization. However, let's verify that this method is properly guarded at the controller/resolver level.
✅ Verification successful
Properly guarded endpoint with role-based access and existence checks
The implementation is properly secured at the controller level with:
@UseGuards(SubpoenaExistsGuard)for validating subpoena existence- Role-based access control through
@RolesRulesdecorator with multiple roles (districtCourtJudge, districtCourtAssistant, districtCourtRegistrar) - User context through
@CurrentHttpUser()decorator - Subpoena context through
@CurrentSubpoena()decorator
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the getSubpoena method is properly guarded in controllers/resolvers
ast-grep --pattern 'class $_ {
$$$
@$_Guard()
$$$
getSubpoena($_, $_) {
$$$
}
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Let's search for files that might contain the controller/resolver using this service
rg -l "SubpoenaService" --type ts
# Then let's look for any endpoint or resolver that calls getSubpoena
rg "getSubpoena\(" --type ts -A 5 -B 5
Length of output: 11718
Script:
#!/bin/bash
# Let's check the guards on the controller endpoint
rg -B 10 "getSubpoena\(" apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts
# Also check what SubpoenaExistsGuard does
cat apps/judicial-system/backend/src/app/modules/subpoena/guards/SubpoenaExistsGuard.ts
Length of output: 986
Security&Integrity
Öryggishola - notandi sem hefur aðgang að einhverju máli getur sótt stöðu birtingar fyrir hvaða varnaraðlia sem er í hvaða máli sem er
Ekki sækja stöðu fyrirkalls til RLS ef staða fyrirkalls er til í gagnagrunni
What
Why
Checklist:
Summary by CodeRabbit
New Features
SubpoenaModulefor enhanced subpoena management.getSubpoenamethod to retrieve subpoena details using case and defendant IDs.defendantIdfield to GraphQL queries for case and limited access case.Bug Fixes
LawyersServicefor when a lawyer is not found.Documentation
Refactor
ServiceAnnouncementcomponent.IndictmentOverviewandOverviewcomponents.CaseControllerandCaseService.Tests
InternalSubpoenaControllerandSubpoenaController.Chores