Task #215826 Task #215928 Addition of AclGuard permissions to the modules #938
Conversation
WalkthroughThe recent updates across various modules in the application focus on enhancing security and access control. The changes include the introduction and removal of Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (10)
Files skipped from review as they are similar to previous changes (6)
Additional Context UsedPath-based Instructions (4)
Additional comments not posted (33)
Warning Following problems were encountered
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/src/common/helpers/acl.helper.ts (4 hunks)
- src/src/kit-materials/kit-materials.controller.ts (1 hunks)
- src/src/kit-materials/kit-materials.module.ts (1 hunks)
Additional comments: 8
src/src/kit-materials/kit-materials.module.ts (3)
- 7-8: The imports are correctly added and follow the project's structure. However, ensure that the
UserModuleandAclHelperare properly documented, especially regarding their roles and interactions within theKitMaterialsModule.- 11-11: Adding
UserModuleto the imports ensures that user-related functionalities are accessible within theKitMaterialsModule. This is a good practice for modularity and maintainability.- 13-13: Including
AclHelperin the providers list is essential for implementing access control within the module. This aligns with best practices for securing modules and ensuring that only authorized actions are performed.src/src/kit-materials/kit-materials.controller.ts (2)
- 13-15: The imports for
AclGuard,AclGuardData, andAclHelperare correctly added. This setup is crucial for implementing fine-grained access control within the controller.- 20-23: The constructor injection of
AclHelperis a good practice, promoting dependency inversion and making the controller more testable and maintainable.src/src/common/helpers/acl.helper.ts (3)
- 25-28: Logging the use of cache for role actions is a good practice for debugging. However, ensure that sensitive information is not logged in production environments.
- 300-332: The addition of
doIHaveKitMaterialAccessmethod is crucial for checking access specific to thekit-materialentity. Ensure that the logic aligns with the application's overall access control strategy and that the method is thoroughly tested.- 351-353: Updating the
doIHaveAccessmethod to handle the 'kit-material' entity type is a necessary change for extending ACL functionalities. This update ensures that the access control logic remains centralized and manageable.
| @UseGuards(AclGuard) | ||
| @AclGuardData('kit-material', ['read.own']) | ||
| async list( | ||
| @Req() request: any, | ||
| @Body() body: any, | ||
| @Res() response: any, | ||
| @Param('camp_id') camp_id: number, | ||
| ) { | ||
| if ( | ||
| !(await this.aclHelper.doIHaveAccess( | ||
| request, | ||
| 'kit-material', | ||
| camp_id, | ||
| )) | ||
| ) { | ||
| return response.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Similar to the create method, the list method correctly uses AclGuard and AclGuardData. Again, consider abstracting repetitive access control checks.
YoginiTayade
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/src/common/helpers/acl.helper.ts (4 hunks)
- src/src/references/references.controller.ts (4 hunks)
- src/src/references/references.module.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/src/common/helpers/acl.helper.ts
Additional comments: 6
src/src/references/references.module.ts (3)
- 6-7: The addition of
AclHelperandUserModuleenhances the module's access control capabilities. Ensure that these dependencies are properly documented in the module's documentation to aid future developers in understanding their purpose and usage.- 10-10: The
importsarray now includesUserModule, which is essential for leveraging user-related functionalities within this module. This change aligns with the PR's objective to enhance access control. However, ensure that the inclusion of multipleHasuraModuleimports (one fromsrc/hasura/hasura.moduleand another from../services/hasura/hasura.module) is intentional and not a duplication error. If they are indeed different modules serving distinct purposes, consider renaming one to avoid confusion.- 12-12: The addition of
AclHelperto theprovidersarray is a crucial step in integrating access control mechanisms. This aligns with the PR's objectives. Ensure thatAclHelperis thoroughly tested to prevent any access control-related issues.src/src/references/references.controller.ts (3)
- 26-27: The use of
@UseGuards(AclGuard)and@AclGuardDatain thecreatemethod is a significant improvement for enforcing access control based on user roles and permissions. This aligns with the PR's objectives. However, ensure that theAclGuardDatadecorator's parameters are correctly configured to match the intended access control policies.- 46-47: The application of
@UseGuards(AclGuard)and@AclGuardDatain thecommunityByIdmethod is another step towards securing access to resources. Ensure that the access control policies defined here are in line with the overall security strategy and that they are tested for edge cases and unauthorized access attempts.- 64-65: Implementing
@UseGuards(AclGuard)and@AclGuardDatain theupdatemethod reinforces the security measures for modifying resources. It's crucial to verify that the permissions specified (['edit.own']) accurately reflect the intended access restrictions and that they are consistently applied across similar endpoints.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/src/common/helpers/acl.helper.ts (6 hunks)
- src/src/pcrscores/pcrscores.controller.ts (3 hunks)
- src/src/pcrscores/pcrscores.module.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/src/common/helpers/acl.helper.ts
Additional comments: 9
src/src/pcrscores/pcrscores.module.ts (2)
- 7-7: The import of
AclHelperis correctly added to enable ACL functionality within thePcrscoresModule. This aligns with the objective of enhancing access control.- 12-12: Adding
AclHelperto the providers array is a good practice as it makes the ACL functionality available to the entire module. This is consistent with NestJS's dependency injection system and enhances modularity and maintainability.src/src/pcrscores/pcrscores.controller.ts (7)
- 16-18: The imports for
AclGuard,AclGuardData, andAclHelperare correctly added, enabling the use of ACL functionality within the controller. This is a necessary step for implementing access control.- 24-27: Modifying the constructor to include
aclHelperis a good practice. It allows the controller to useaclHelpermethods for access control checks, aligning with dependency injection principles in NestJS.- 31-32: The use of
AclGuardandAclGuardDatadecorators in thecreatemethod is appropriate for enforcing access control based on user roles and permissions. This ensures that only authorized users can create PCR scores.- 39-40: The commented-out
AclGuardandAclGuardDatadecorators in thepcrscoreListmethod suggest a work-in-progress or a decision to not enforce ACL here. If this is intentional, ensure that the rationale is documented. Otherwise, consider removing the comments or implementing the necessary access control.- 47-48: The application of
AclGuardandAclGuardDatadecorators to thepcrscoreByIdmethod correctly enforces access control, ensuring that users can only access PCR scores they are authorized to view.- 60-79: The implementation of access control in the
pcrscoreByUser_idmethod, including the use ofaclHelperto check access, is well done. This ensures that users can only access PCR scores associated with their user ID, enhancing security.- 91-92: Using
AclGuardandAclGuardDatadecorators in theupdatemethod is correct for enforcing access control on PCR score updates. This ensures that users can only edit PCR scores they are authorized to modify.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/src/common/helpers/acl.helper.ts (6 hunks)
- src/src/sessions/sessions.controller.ts (6 hunks)
- src/src/sessions/sessions.module.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/src/common/helpers/acl.helper.ts
Additional comments: 6
src/src/sessions/sessions.module.ts (1)
- 12-12: The addition of
AclHelperto theSessionsModuleproviders is a positive step towards enhancing access control. Ensure thatAclHelperis thoroughly integrated and tested within the module to confirm its functionality.Verification successful
The addition of
AclHelperto theSessionsModuleproviders has been confirmed, withAclHelpercorrectly located within the project. This supports the initial review comment on enhancing access control. It remains important to ensure thatAclHelperis thoroughly integrated and tested within the module to confirm its functionality and impact.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that AclHelper is correctly imported and no dependency issues are introduced. rg 'AclHelper' src/common/helpers/acl.helper.ts # Ensure that AclHelper is utilized within SessionsModule or its services. ast-grep --lang typescript --pattern $'class SessionsModule { $$$ constructor($_: AclHelper) { $$$ } $$$ }'Length of output: 241
Script:
#!/bin/bash # Broad search for AclHelper to confirm its existence and correct path. rg 'export class AclHelper' --files-with-matches # Broaden the search for AclHelper usage within SessionsModule and its services. rg 'AclHelper' src/src/sessions --files-with-matchesLength of output: 176
src/src/sessions/sessions.controller.ts (5)
- 17-18: The import of
AclGuardandAclGuardDatais correctly done, aligning with the PR's objective to enhance access control. Ensure these are consistently used across all relevant methods.- 27-28: The use of
AclGuardandAclGuardDatain thecreateSessionmethod is appropriate for enforcing access control. Verify that the permissions ('create') align with the intended access levels for this operation.- 40-41: The
updateSessionmethod correctly usesAclGuardandAclGuardDatafor access control. Ensure the 'edit.own' permission is consistent with the business logic for session updates.Verification successful
The
updateSessionmethod is correctly decorated with@UseGuards(AclGuard)and@AclGuardDatausing the 'edit.own' permission, as initially reviewed. This confirms the use of the specified permission for access control in session updates is consistent with the business logic.* 71-72: The `getSessionDetailsById` method's use of `AclGuard` and `AclGuardData` with 'read.own' permission is appropriate for access control. Confirm that this permission setting aligns with the access requirements for session details retrieval.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'edit.own' permission is consistent with the business logic for session updates. rg 'updateSession' -A 10 | grep '@AclGuardData'Length of output: 127
Script:
#!/bin/bash # Capture the context around the 'updateSession' method to verify the actual permission used. rg 'updateSession' -A 10 -B 10 src/src/sessions/sessions.controller.tsLength of output: 724
Verification successful
The output confirms the use of
@UseGuards(AclGuard)and@AclGuardData('session', ['read.own'])directly above thegetSessionDetailsByIdmethod insessions.controller.ts. This setup indicates that the method is protected by access control mechanisms, specifically requiring 'read.own' permission for access. Given this context, it appears that the initial review comment is aligned with the codebase, as it correctly identifies the decorators and the permission setting used for thegetSessionDetailsByIdmethod.* 90-91: The `getSessionDetailsByIdGetOne` method's implementation of `AclGuard` and `AclGuardData` with 'read.own' permission is consistent with access control best practices. Validate that this permission setting is suitable for the operation's access level.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'read.own' permission aligns with the access requirements for session details retrieval. rg 'getSessionDetailsById' -A 10 | grep '@AclGuardData'Length of output: 53
Script:
#!/bin/bash # Search for the getSessionDetailsById method to confirm the use of AclGuard and AclGuardData decorators rg 'getSessionDetailsById' -A 20 -B 2Length of output: 4801
Verification successful
The
getSessionDetailsByIdGetOnemethod's implementation ofAclGuardandAclGuardDatawith 'read.own' permission is confirmed to be consistent with access control best practices, as evidenced by the decorators directly associated with the method in the controller. This setup ensures that only authenticated users with the appropriate permissions can access their own session details, aligning with the intended access level for this operation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'read.own' permission is suitable for the operation's access level in getSessionDetailsByIdGetOne. rg 'getSessionDetailsByIdGetOne' -A 10 | grep '@AclGuardData'Length of output: 59
Script:
#!/bin/bash # Confirm the presence of 'getSessionDetailsByIdGetOne' and its decorators in the codebase. rg 'getSessionDetailsByIdGetOne' -A 3 -B 3Length of output: 1225
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/src/common/helpers/acl.helper.ts (6 hunks)
- src/src/upload-file/upload-file.controller.ts (5 hunks)
- src/src/upload-file/upload-file.module.ts (1 hunks)
Additional comments: 5
src/src/upload-file/upload-file.module.ts (1)
- 7-13: The addition of
AclHelperto providers andUserModuleto imports inUploadFileModuleis correctly implemented, adhering to NestJS module structure and best practices.src/src/upload-file/upload-file.controller.ts (3)
- 17-18: The addition of imports for
AclGuardandAclGuardDatais appropriate for implementing access control mechanisms.- 27-29: The use of
@UseGuards(AclGuard)and@AclGuardDatadecorators to enforce access control on methods is correctly implemented, following NestJS best practices for security.Also applies to: 50-51, 73-74
- 61-62: The decorators
@UseGuards(AclGuard)and@AclGuardDataare commented out for thegetFileUrlmethod. Please clarify if this is intentional and whether access control is not required for this endpoint.src/src/common/helpers/acl.helper.ts (1)
- 299-476: Ensure that all new access check methods correctly handle permissions and access control logic, especially in terms of performance and security. It's important to validate that these methods do not introduce any security vulnerabilities or performance bottlenecks.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/src/common/helpers/acl.helper.ts (6 hunks)
- src/src/modules/edit-requests/edit-requests.controller.ts (5 hunks)
- src/src/modules/edit-requests/edit-requests.module.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/src/common/helpers/acl.helper.ts
Additional comments: 5
src/src/modules/edit-requests/edit-requests.module.ts (1)
- 14-14: The addition of
AclHelperto the providers list inEditRequestModuleis a crucial step towards implementing enhanced access control mechanisms. Ensure thatAclHelperis correctly implemented and its dependencies are adequately met within the module.src/src/modules/edit-requests/edit-requests.controller.ts (4)
- 25-26: The
AclGuardandAclGuardDatadecorators are commented out in thegetEditRequestsListmethod. If this is intentional for development or testing purposes, consider documenting this decision or tracking it in a task to ensure they are enabled when appropriate. Commented-out code should generally be avoided in production code.- 41-42: The use of
AclGuardandAclGuardDatadecorators in thecreateEditRequestsmethod is appropriate for enforcing access control. Ensure that the specified permissions (create) are correctly defined and implemented.- 57-58: The use of
AclGuardandAclGuardDatadecorators in theupdateEditRequestsmethod is correctly applied for access control. Verify that theedit.ownpermission is properly defined and enforced.- 75-76: The use of
AclGuardandAclGuardDatadecorators in thegetAdminEditRequestsmethod is appropriate for enforcing access control. Ensure that theread.ownpermission is correctly defined and implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (6)
- src/src/camp/camp.controller.ts (23 hunks)
- src/src/common/helpers/acl.helper.ts (6 hunks)
- src/src/events/events.controller.ts (11 hunks)
- src/src/events/events.module.ts (3 hunks)
- src/src/facilitator/facilitator.controller.ts (12 hunks)
- src/src/facilitator/facilitator.module.ts (2 hunks)
Additional Context Used
Path-based Instructions (6)
src/src/events/events.module.ts (1)
Pattern
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/facilitator/facilitator.module.ts (1)
Pattern
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/events/events.controller.ts (1)
Pattern
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/facilitator/facilitator.controller.ts (1)
Pattern
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/camp/camp.controller.ts (1)
Pattern
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/common/helpers/acl.helper.ts (1)
Pattern
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Additional comments not posted (7)
src/src/events/events.module.ts (2)
15-15: EnsureAclHelperis properly injected and utilized within the module.
AddingAclHelperto the providers array is a crucial step for enabling access control functionalities across theEventsModule. This change allows theEventsServiceand controllers within this module to leverageAclHelperfor access control checks, enhancing the security of the application.
37-46: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [22-53]
Review the usage of
CohortMiddlewareand ensure the excluded routes are correctly configured.
The configuration ofCohortMiddlewarewith specific routes excluded is a critical aspect of controlling access to certain endpoints. It's important to verify that the paths and methods specified in theexcludemethod accurately reflect the intended access control policies for the application. Additionally, consider documenting the rationale behind excluding these specific routes to aid future maintenance and understanding of the access control logic.src/src/facilitator/facilitator.module.ts (1)
22-22: EnsureAclHelperis properly injected and utilized within the module.
AddingAclHelperto the providers array enables access control functionalities across theFacilitatorModule. This integration is essential for enforcing security policies and ensuring that only authorized users can access or modify facilitator-related data.src/src/events/events.controller.ts (1)
22-23: Ensure theAclGuardandAclGuardDatadecorators are correctly implemented.
The addition ofAclGuardandAclGuardDatadecorators to various endpoints in theEventsControlleris a significant enhancement to the application's security. These decorators enforce access control checks, ensuring that only authorized users can perform specific actions. It's important to verify that the permissions specified inAclGuardDataaccurately reflect the intended access control policies for each endpoint.src/src/facilitator/facilitator.controller.ts (1)
22-24: Ensure theAclGuard,AclGuardData, andAclHelperare correctly implemented and utilized.
The integration ofAclGuardandAclGuardDatadecorators, along with the injection ofAclHelperin the constructor, significantly strengthens the access control mechanisms of theFacilitatorController. These changes ensure that access to facilitator-related actions is properly guarded based on defined permissions. It's crucial to verify that the permissions specified inAclGuardDataaccurately match the intended access control policies for each action.src/src/camp/camp.controller.ts (1)
90-100: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [93-145]
Ensure the
AclGuardandAclGuardDatadecorators are correctly implemented for camp-related actions.
The replacement ofRoleGuardwithAclGuardand the specification of permissions usingAclGuardDatadecorators for various camp-related actions enhance the application's security by enforcing granular access control. It's important to verify that the permissions specified accurately reflect the intended access control policies for each action, ensuring that only authorized users can perform specific operations on camps.src/src/common/helpers/acl.helper.ts (1)
299-540: Validate the implementation of new access check methods.
The addition of new private methods (doIHaveKitMaterialAccess,doIHaveReferenceAccess, etc.) for specific access checks and the update to thedoIHaveAccessmethod to handle additional entity types are correctly implemented, enhancing the modularity and maintainability of the access control logic. Ensure that these methods are thoroughly tested to confirm their correctness and effectiveness in enforcing access control policies.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/src/common/helpers/acl.helper.ts (6 hunks)
- src/src/user/user.controller.ts (10 hunks)
- src/src/user/user.module.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/src/common/helpers/acl.helper.ts
Additional Context Used
Path-based Instructions (2)
src/src/user/user.module.ts (1)
Pattern
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/user/user.controller.ts (1)
Pattern
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Additional comments not posted (9)
src/src/user/user.module.ts (1)
11-11: AddingAclHelperto the providers enhances ACL support. Ensure all dependencies are correctly managed.src/src/user/user.controller.ts (8)
91-93: Correct use ofAclGuardandAclGuardDatafor access control. Verify permissions align with the access control policy.
103-105: Correct application ofAclGuardandAclGuardData. Ensureread.ownpermission is consistent with intended access control.
115-116: Consistent use ofAclGuardandAclGuardData. Verifyread.ownpermission for access control accuracy.
143-144: Proper use ofAclGuardandAclGuardDatafor user updates. Confirmedit.ownpermission matches access control requirements.
206-207: Appropriate application ofAclGuardandAclGuardDatafor audit logs. Ensureread.ownpermission is correctly defined.
224-225: Consistent implementation ofAclGuardandAclGuardData. Verifyread.ownpermission for access control integrity.
258-259: Proper use ofAclGuardandAclGuardDatafor IP list access. Confirmreadpermission aligns with access control plans.
270-271: Effective application ofAclGuardandAclGuardDatafor cohort IP list access. Ensurereadpermission is accurately defined.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- src/src/common/helpers/acl.helper.ts (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/src/common/helpers/acl.helper.ts
src/src/camp/camp.controller.ts
Outdated
| // @UseGuards(RoleGuard) | ||
| // @Roles('staff', 'program_owner') | ||
| @UseGuards(AclGuard) | ||
| @AclGuardData('camp', ['create']) |
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (1)
src/src/camp/camp.service.ts (1)
Line range hint
58-250: Consider refactoring thecreatemethod to improve readability and maintainability.The
createmethod is quite lengthy and handles multiple responsibilities, which makes it hard to read and maintain. Consider breaking it down into smaller, more focused methods. For example, separate the validation logic, database interactions, and response handling into different methods or services. This will not only improve readability but also enhance the testability of the code.
src/src/camp/camp.service.ts
Outdated
| console.log(query); | ||
|
|
There was a problem hiding this comment.
Consider removing the console.log statement for production readiness.
- console.log(query);This logging statement could potentially expose sensitive query details in production logs, which might include user data or other sensitive information. It's generally a good practice to remove or replace such logging with more secure logging mechanisms before deploying to production.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| console.log(query); | |
| public async updateAttendanceDetail( | ||
| id: number, | ||
| req: any, | ||
| response: any, | ||
| request: any, | ||
| ) { | ||
| const user_roles = request.mw_roles; | ||
| console.log(user_roles); | ||
|
|
||
| if ( | ||
| user_roles.includes('facilitator') || | ||
| user_roles.includes('program_owner') | ||
| ) { | ||
| return response.status(403).json({ | ||
| status: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } else if (user_roles.includes('staff')) { | ||
| let hasura_query = { | ||
| query: `query MyQuery { | ||
| attendance(where: {id: {_eq: ${id}}}) { | ||
| context_id | ||
| } | ||
| }`, | ||
| }; | ||
|
|
||
| const hasura_response = | ||
| await this.hasuraServiceFromServices.getData(hasura_query); | ||
|
|
||
| let events_id; | ||
| if ( | ||
| !hasura_response?.data && | ||
| hasura_response.data.attendance[0].context_id | ||
| ) { | ||
| return response.status(403).json({ | ||
| status: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } else { | ||
| events_id = hasura_response.data.attendance[0].context_id; | ||
| let gqlquery = { | ||
| query: `query MyQuery2 { | ||
| events_aggregate(where: {id: {_eq: ${events_id}}, user_id: {_eq: ${request.mw_userid}}}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| }`, | ||
| }; | ||
| console.log(gqlquery.query); | ||
|
|
||
| const result = await this.hasuraServiceFromServices.getData( | ||
| gqlquery, | ||
| ); | ||
|
|
||
| if ( | ||
| !result?.data && | ||
| result.data.events_aggregate.aggregate.count > 0 | ||
| ) { | ||
| return response.status(403).json({ | ||
| status: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Review the logic for role-based access control in updateAttendanceDetail.
The method updateAttendanceDetail has been updated to include role-based access control. However, there are several issues and potential improvements:
- Logic Error in Role Check: The condition on lines 613-615 seems incorrect. It checks if
hasura_response.datais falsy and then tries to accesshasura_response.data.attendance[0].context_id. This will throw an error ifhasura_response.datais undefined or null. - Redundant Logging: The
console.logon line 589 and 633 might expose sensitive query details in production logs. Consider removing or replacing them with more secure logging mechanisms. - Performance Consideration: The method makes multiple asynchronous calls to the database within conditional blocks. Consider optimizing these queries or combining them to reduce the number of calls.
- Security Enhancement: Ensure that all user inputs used in queries are properly sanitized to prevent SQL injection attacks. This is particularly important for any IDs or user-generated strings that might be included in the GraphQL queries.
Consider addressing these issues to improve the robustness and security of the role-based access control implementation.
| const user_roles = request.mw_roles; | ||
| let gqlquery; | ||
| if (user_roles.includes('facilitator')) { | ||
| return response.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } else if (user_roles.includes('staff')) { | ||
| const parent_ip_data = await this.userService.getIpRoleUserById( | ||
| request.mw_userid, | ||
| { | ||
| program_id: request.mw_program_id, | ||
| academic_year_id: request.mw_academic_year, | ||
| }, | ||
| ); | ||
|
|
||
| const parent_ip_id = | ||
| parent_ip_data?.program_users?.[0]?.organisation_id; | ||
| gqlquery = { | ||
| query: `query MyQuery { | ||
| program_faciltators_aggregate(where: {program_id: {_eq: ${request.mw_program_id}}, academic_year_id: {_eq: ${request.mw_academic_year_id}}, parent_ip: {_eq: "${parent_ip_id}"}}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| }`, | ||
| }; | ||
| console.log(gqlquery.query); | ||
|
|
||
| const result = await this.hasuraServiceFromServices.getData( | ||
| gqlquery, | ||
| ); | ||
| if ( | ||
| !result?.data && | ||
| result.data.program_faciltators_aggregate.aggregate.count > 0 | ||
| ) { | ||
| return response.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper handling of GraphQL query results.
The logic in lines 110-113 seems incorrect. The condition checks if result?.data is falsy and then tries to access result.data.program_faciltators_aggregate.aggregate.count, which would cause a runtime error if result.data is indeed falsy. Consider revising the condition to ensure that result.data is truthy before accessing its properties.
- if (!result?.data && result.data.program_faciltators_aggregate.aggregate.count > 0)
+ if (result?.data && result.data.program_faciltators_aggregate.aggregate.count > 0)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const user_roles = request.mw_roles; | |
| let gqlquery; | |
| if (user_roles.includes('facilitator')) { | |
| return response.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } else if (user_roles.includes('staff')) { | |
| const parent_ip_data = await this.userService.getIpRoleUserById( | |
| request.mw_userid, | |
| { | |
| program_id: request.mw_program_id, | |
| academic_year_id: request.mw_academic_year, | |
| }, | |
| ); | |
| const parent_ip_id = | |
| parent_ip_data?.program_users?.[0]?.organisation_id; | |
| gqlquery = { | |
| query: `query MyQuery { | |
| program_faciltators_aggregate(where: {program_id: {_eq: ${request.mw_program_id}}, academic_year_id: {_eq: ${request.mw_academic_year_id}}, parent_ip: {_eq: "${parent_ip_id}"}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| }`, | |
| }; | |
| console.log(gqlquery.query); | |
| const result = await this.hasuraServiceFromServices.getData( | |
| gqlquery, | |
| ); | |
| if ( | |
| !result?.data && | |
| result.data.program_faciltators_aggregate.aggregate.count > 0 | |
| ) { | |
| return response.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } | |
| } | |
| if ( | |
| result?.data && result.data.program_faciltators_aggregate.aggregate.count > 0 | |
| ) { | |
| return response.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } |
| const user_roles = req.mw_roles; | ||
| let gqlquery; | ||
| if (user_roles.includes('facilitator')) { | ||
| const parent_ip_query = { | ||
| query: `query MyQuery { | ||
| program_faciltators(where: {user_id: {_eq: ${req.mw_userid}}, program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}}) { | ||
| parent_ip | ||
| } | ||
| } | ||
| `, | ||
| }; | ||
| console.log(parent_ip_query.query); | ||
|
|
||
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | ||
| parent_ip_query, | ||
| ); | ||
| const parent_ip_id = | ||
| parent_ip_data.data.program_faciltators[0].parent_ip; | ||
|
|
||
| gqlquery = { | ||
| query: `query MyQuery { | ||
| program_faciltators_aggregate(where: {program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}, parent_ip: {_eq: "${parent_ip_id}"}}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| }; | ||
| console.log(gqlquery.query); | ||
|
|
||
| const result = await this.hasuraServiceFromServices.getData( | ||
| gqlquery, | ||
| ); | ||
| if ( | ||
| !result?.data && | ||
| result.data.program_faciltators_aggregate.aggregate.count > 0 | ||
| ) { | ||
| return resp.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Review the logic for handling GraphQL query results.
Similar to the previous comment, the condition in lines 448-451 is potentially erroneous. It should ensure that result.data is truthy before attempting to access result.data.program_faciltators_aggregate.aggregate.count.
- if (!result?.data && result.data.program_faciltators_aggregate.aggregate.count > 0)
+ if (result?.data && result.data.program_faciltators_aggregate.aggregate.count > 0)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const user_roles = req.mw_roles; | |
| let gqlquery; | |
| if (user_roles.includes('facilitator')) { | |
| const parent_ip_query = { | |
| query: `query MyQuery { | |
| program_faciltators(where: {user_id: {_eq: ${req.mw_userid}}, program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}}) { | |
| parent_ip | |
| } | |
| } | |
| `, | |
| }; | |
| console.log(parent_ip_query.query); | |
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | |
| parent_ip_query, | |
| ); | |
| const parent_ip_id = | |
| parent_ip_data.data.program_faciltators[0].parent_ip; | |
| gqlquery = { | |
| query: `query MyQuery { | |
| program_faciltators_aggregate(where: {program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}, parent_ip: {_eq: "${parent_ip_id}"}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| console.log(gqlquery.query); | |
| const result = await this.hasuraServiceFromServices.getData( | |
| gqlquery, | |
| ); | |
| if ( | |
| !result?.data && | |
| result.data.program_faciltators_aggregate.aggregate.count > 0 | |
| ) { | |
| return resp.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } | |
| const user_roles = req.mw_roles; | |
| let gqlquery; | |
| if (user_roles.includes('facilitator')) { | |
| const parent_ip_query = { | |
| query: `query MyQuery { | |
| program_faciltators(where: {user_id: {_eq: ${req.mw_userid}}, program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}}) { | |
| parent_ip | |
| } | |
| } | |
| `, | |
| }; | |
| console.log(parent_ip_query.query); | |
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | |
| parent_ip_query, | |
| ); | |
| const parent_ip_id = | |
| parent_ip_data.data.program_faciltators[0].parent_ip; | |
| gqlquery = { | |
| query: `query MyQuery { | |
| program_faciltators_aggregate(where: {program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}, parent_ip: {_eq: "${parent_ip_id}"}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| console.log(gqlquery.query); | |
| const result = await this.hasuraServiceFromServices.getData( | |
| gqlquery, | |
| ); | |
| if ( | |
| result?.data && result.data.program_faciltators_aggregate.aggregate.count > 0 | |
| ) { | |
| return resp.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } |
| const parent_id = parent_ip.program_users[0].organisation_id; | ||
| gqlquery = { | ||
| query: `query MyQuery { | ||
| program_beneficiaries_aggregate ( | ||
| where: { | ||
| facilitator_user: { | ||
| program_faciltators: { | ||
| ${ | ||
| req.academic_year_id | ||
| ? `academic_year_id: {_eq: ${req.mw_academic_year_id}}` | ||
| : `` | ||
| }, | ||
| ${req.program_id ? `program_id: {_eq: ${req.mw_program_id}}` : ``}, | ||
| parent_ip: {_eq: "${parent_id}"} | ||
| } | ||
| }, | ||
| ${ | ||
| req.academic_year_id | ||
| ? `academic_year_id: {_eq: ${req.mw_academic_year_id}}` | ||
| : `` | ||
| }, | ||
| ${req.program_id ? `program_id: {_eq: ${req.mw_program_id}}` : ``} | ||
| } | ||
| ) { | ||
| aggregate{ | ||
| count | ||
| } | ||
| } | ||
| }`, | ||
| }; | ||
| console.log(gqlquery.query); | ||
| const result = await this.hasuraServiceFromServices.getData( | ||
| gqlquery, | ||
| ); | ||
| if ( |
There was a problem hiding this comment.
Optimize GraphQL query construction for readability and maintainability.
- gqlquery = {
- query: `query MyQuery {
- program_beneficiaries_aggregate (
- where: {
- facilitator_user: {
- program_faciltators: {
- ${
- req.academic_year_id
- ? `academic_year_id: {_eq: ${req.mw_academic_year_id}}`
- : ``
- },
- ${req.program_id ? `program_id: {_eq: ${req.mw_program_id}}` : ``},
- parent_ip: {_eq: "${parent_id}"}
- }
- },
- ${
- req.academic_year_id
- ? `academic_year_id: {_eq: ${req.mw_academic_year_id}}`
- : ``
- },
- ${req.program_id ? `program_id: {_eq: ${req.mw_program_id}}` : ``}
- }
- ) {
- aggregate{
- count
- }
- }
- }`,
- };
+ gqlquery = this.constructBeneficiaryAggregateQuery(req, parent_id);Refactor the GraphQL query construction into a separate method to improve readability and reusability.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const parent_id = parent_ip.program_users[0].organisation_id; | |
| gqlquery = { | |
| query: `query MyQuery { | |
| program_beneficiaries_aggregate ( | |
| where: { | |
| facilitator_user: { | |
| program_faciltators: { | |
| ${ | |
| req.academic_year_id | |
| ? `academic_year_id: {_eq: ${req.mw_academic_year_id}}` | |
| : `` | |
| }, | |
| ${req.program_id ? `program_id: {_eq: ${req.mw_program_id}}` : ``}, | |
| parent_ip: {_eq: "${parent_id}"} | |
| } | |
| }, | |
| ${ | |
| req.academic_year_id | |
| ? `academic_year_id: {_eq: ${req.mw_academic_year_id}}` | |
| : `` | |
| }, | |
| ${req.program_id ? `program_id: {_eq: ${req.mw_program_id}}` : ``} | |
| } | |
| ) { | |
| aggregate{ | |
| count | |
| } | |
| } | |
| }`, | |
| }; | |
| console.log(gqlquery.query); | |
| const result = await this.hasuraServiceFromServices.getData( | |
| gqlquery, | |
| ); | |
| if ( | |
| const parent_id = parent_ip.program_users[0].organisation_id; | |
| gqlquery = this.constructBeneficiaryAggregateQuery(req, parent_id); | |
| console.log(gqlquery.query); | |
| const result = await this.hasuraServiceFromServices.getData( | |
| gqlquery, | |
| ); | |
| if ( |
| const parent_ip = await this.userService.getIpRoleUserById( | ||
| req.mw_userid, | ||
| { | ||
| program_id: req.mw_program_id, | ||
| academic_year_id: req.mw_academic_year_id, | ||
| }, | ||
| ); | ||
| console.log(JSON.stringify(parent_ip)); | ||
|
|
||
| const parent_id = parent_ip.program_users[0].organisation_id; | ||
| gqlquery = { | ||
| query: `query MyQuery { | ||
| program_beneficiaries_aggregate ( | ||
| where: { | ||
| facilitator_user: { | ||
| program_faciltators: { | ||
| ${ | ||
| req.academic_year_id | ||
| ? `academic_year_id: {_eq: ${req.mw_academic_year_id}}` | ||
| : `` | ||
| }, | ||
| ${req.program_id ? `program_id: {_eq: ${req.mw_program_id}}` : ``}, | ||
| parent_ip: {_eq: "${parent_id}"} | ||
| } | ||
| }, | ||
| ${ | ||
| req.academic_year_id | ||
| ? `academic_year_id: {_eq: ${req.mw_academic_year_id}}` | ||
| : `` | ||
| }, | ||
| ${req.program_id ? `program_id: {_eq: ${req.mw_program_id}}` : ``} | ||
| } | ||
| ) { | ||
| aggregate{ | ||
| count | ||
| } | ||
| } | ||
| }`, | ||
| }; | ||
| console.log(gqlquery.query); | ||
| const result = await this.hasuraServiceFromServices.getData( | ||
| gqlquery, | ||
| ); | ||
| if ( |
There was a problem hiding this comment.
Optimize and abstract GraphQL query construction.
Similar to the previous method, refactor the GraphQL query construction into a separate method to enhance maintainability and avoid code duplication.
src/src/camp/camp.service.ts
Outdated
| const user_roles = req.mw_roles; | ||
| let gqlquery; | ||
| if (user_roles.includes('facilitator')) { | ||
| let filter; | ||
| if (req.mw_program_id && req.mw_academic_year_id) { | ||
| filter = `{groups: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}},`; | ||
| } else { | ||
| filter = ``; | ||
| } | ||
| const parent_ip_query = { | ||
| query: `query MyQuery { | ||
| program_faciltators(where: {${filter} user_id: {_eq: ${req.mw_userid}}}) { | ||
| parent_ip | ||
| } | ||
| } | ||
| `, | ||
| }; | ||
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | ||
| parent_ip_query, | ||
| ); | ||
| const parent_ip = | ||
| parent_ip_data.data.program_faciltators[0].parent_ip; | ||
| gqlquery = { | ||
| query: `query MyQuery { | ||
| camps_aggregate(where: {group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, user: {${filter} group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}, user_id: {_eq: ${req.mw_userid}}}, program_faciltators: {parent_ip: {_eq: "${parent_ip}"}}}}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| } | ||
|
|
||
| `, | ||
| }; | ||
| } else if (user_roles.includes('staff')) { | ||
| let filter; | ||
| if (req.mw_program_id && req.mw_academic_year_id) { | ||
| filter = `{groups: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}},`; | ||
| } else { | ||
| filter = ``; | ||
| } | ||
| const parent_ip_query = { | ||
| query: `query MyQuery { | ||
| program_faciltators(where: {${filter} user_id: {_eq: ${req.mw_userid}}}) { | ||
| parent_ip | ||
| } | ||
| } | ||
| `, | ||
| }; | ||
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | ||
| parent_ip_query, | ||
| ); | ||
| const parent_ip = | ||
| parent_ip_data.data.program_faciltators[0].parent_ip; | ||
| gqlquery = { | ||
| query: `query MyQuery { | ||
| camps_aggregate(where: {group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, user: {${filter} group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, program_faciltators: {parent_ip: {_eq: "${parent_ip}"}}}}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| } | ||
|
|
||
| `, | ||
| }; | ||
| } | ||
| console.log(gqlquery.query); | ||
|
|
||
| const result = await this.hasuraServiceFromServices.getData(gqlquery); | ||
|
|
||
| if ( | ||
| !(result?.data && result.data.camps_aggregate.aggregate.count > 0) | ||
| ) { | ||
| return resp.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The method getStatuswiseCount should include error handling for the asynchronous calls to external services to enhance reliability.
- const result = await this.hasuraServiceFromServices.getData(gqlquery);
+ try {
+ const result = await this.hasuraServiceFromServices.getData(gqlquery);
+ // existing logic
+ } catch (error) {
+ return resp.status(500).json({
+ success: false,
+ message: 'Error fetching data',
+ error: error.message,
+ });
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const user_roles = req.mw_roles; | |
| let gqlquery; | |
| if (user_roles.includes('facilitator')) { | |
| let filter; | |
| if (req.mw_program_id && req.mw_academic_year_id) { | |
| filter = `{groups: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}},`; | |
| } else { | |
| filter = ``; | |
| } | |
| const parent_ip_query = { | |
| query: `query MyQuery { | |
| program_faciltators(where: {${filter} user_id: {_eq: ${req.mw_userid}}}) { | |
| parent_ip | |
| } | |
| } | |
| `, | |
| }; | |
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | |
| parent_ip_query, | |
| ); | |
| const parent_ip = | |
| parent_ip_data.data.program_faciltators[0].parent_ip; | |
| gqlquery = { | |
| query: `query MyQuery { | |
| camps_aggregate(where: {group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, user: {${filter} group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}, user_id: {_eq: ${req.mw_userid}}}, program_faciltators: {parent_ip: {_eq: "${parent_ip}"}}}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| } else if (user_roles.includes('staff')) { | |
| let filter; | |
| if (req.mw_program_id && req.mw_academic_year_id) { | |
| filter = `{groups: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}},`; | |
| } else { | |
| filter = ``; | |
| } | |
| const parent_ip_query = { | |
| query: `query MyQuery { | |
| program_faciltators(where: {${filter} user_id: {_eq: ${req.mw_userid}}}) { | |
| parent_ip | |
| } | |
| } | |
| `, | |
| }; | |
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | |
| parent_ip_query, | |
| ); | |
| const parent_ip = | |
| parent_ip_data.data.program_faciltators[0].parent_ip; | |
| gqlquery = { | |
| query: `query MyQuery { | |
| camps_aggregate(where: {group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, user: {${filter} group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, program_faciltators: {parent_ip: {_eq: "${parent_ip}"}}}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| } | |
| console.log(gqlquery.query); | |
| const result = await this.hasuraServiceFromServices.getData(gqlquery); | |
| if ( | |
| !(result?.data && result.data.camps_aggregate.aggregate.count > 0) | |
| ) { | |
| return resp.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } | |
| const user_roles = req.mw_roles; | |
| let gqlquery; | |
| if (user_roles.includes('facilitator')) { | |
| let filter; | |
| if (req.mw_program_id && req.mw_academic_year_id) { | |
| filter = `{groups: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}},`; | |
| } else { | |
| filter = ``; | |
| } | |
| const parent_ip_query = { | |
| query: `query MyQuery { | |
| program_faciltators(where: {${filter} user_id: {_eq: ${req.mw_userid}}}) { | |
| parent_ip | |
| } | |
| } | |
| `, | |
| }; | |
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | |
| parent_ip_query, | |
| ); | |
| const parent_ip = | |
| parent_ip_data.data.program_faciltators[0].parent_ip; | |
| gqlquery = { | |
| query: `query MyQuery { | |
| camps_aggregate(where: {group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, user: {${filter} group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}, user_id: {_eq: ${req.mw_userid}}}, program_faciltators: {parent_ip: {_eq: "${parent_ip}"}}}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| } else if (user_roles.includes('staff')) { | |
| let filter; | |
| if (req.mw_program_id && req.mw_academic_year_id) { | |
| filter = `{groups: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}},`; | |
| } else { | |
| filter = ``; | |
| } | |
| const parent_ip_query = { | |
| query: `query MyQuery { | |
| program_faciltators(where: {${filter} user_id: {_eq: ${req.mw_userid}}}) { | |
| parent_ip | |
| } | |
| } | |
| `, | |
| }; | |
| const parent_ip_data = await this.hasuraServiceFromServices.getData( | |
| parent_ip_query, | |
| ); | |
| const parent_ip = | |
| parent_ip_data.data.program_faciltators[0].parent_ip; | |
| gqlquery = { | |
| query: `query MyQuery { | |
| camps_aggregate(where: {group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, user: {${filter} group_users: {member_type: {_eq: "owner"}, status: {_eq: "active"}}, program_faciltators: {parent_ip: {_eq: "${parent_ip}"}}}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| } | |
| console.log(gqlquery.query); | |
| try { | |
| const result = await this.hasuraServiceFromServices.getData(gqlquery); | |
| if ( | |
| !(result?.data && result.data.camps_aggregate.aggregate.count > 0) | |
| ) { | |
| return resp.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } | |
| } catch (error) { | |
| return resp.status(500).json({ | |
| success: false, | |
| message: 'Error fetching data', | |
| error: error.message, | |
| }); | |
| } |
| console.log( | ||
| '### Using Cache for roleActions: ' + | ||
| JSON.stringify(roleActions), | ||
| ); |
There was a problem hiding this comment.
Replace console.log with a more robust logging mechanism.
- console.log(...)
+ // Consider replacing console.log with a more robust logging mechanismCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| console.log( | |
| '### Using Cache for roleActions: ' + | |
| JSON.stringify(roleActions), | |
| ); | |
| // Consider replacing console.log with a more robust logging mechanism | |
| console.log( | |
| '### Using Cache for roleActions: ' + | |
| JSON.stringify(roleActions), | |
| ); |
| // @UseGuards(AclGuard) | ||
| // @AclGuardData('kit-material', ['read.own']) | ||
| async list( | ||
| @Req() request: any, | ||
| @Body() body: any, | ||
| @Res() response: any, | ||
| @Param('camp_id') camp_id: number, | ||
| ) { | ||
| if ( | ||
| !(await this.aclHelper.doIHaveAccess( | ||
| request, | ||
| 'kit-material', | ||
| camp_id, | ||
| )) | ||
| ) { | ||
| return response.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The provided script output confirms that the ACL guards (@UseGuards(AclGuard) and @AclGuardData('kit-material', ['read.own'])) are indeed commented out in the list method of the kit-materials.controller.ts file. The presence of these commented lines directly above the list method, along with the active @UseGuards(AuthGuard), suggests that the ACL guards were considered but not enabled.
Since the script output does not provide any comments or documentation explaining why these guards are commented out, it is unclear whether this was an intentional decision for the current implementation or an oversight.
Given the lack of explicit documentation or comments in the vicinity of the commented code explaining the reason for this decision, it is advisable to either:
- Provide documentation within the code explaining why these guards are commented out if it was an intentional decision.
- Uncomment and configure these guards if they are meant to be part of the final implementation.
This ensures clarity for future maintenance and aligns with best practices for code readability and security management.
Conclusion:
- The ACL guards are commented out without accompanying explanations.
- It is recommended to address this either by documenting the reason for the guards being disabled or by enabling them if they are required for the application's security.
Analysis chain
The list method has commented-out ACL guards. If these are intentionally commented out, consider adding a comment explaining the reason. If they are meant to be part of the final implementation, ensure they are uncommented and properly configured.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the ACL guards are intentionally commented out in the `list` method.
cat src/src/kit-materials/kit-materials.controller.ts | grep -A 5 -B 5 "@Get('/list/:camp_id')"
Length of output: 341
src/src/events/events.controller.ts
Outdated
| @UseGuards(AuthGuard) | ||
| // @UseGuards(AclGuard) | ||
| // @AclGuardData('event', ['read.own']) |
There was a problem hiding this comment.
Tip
Codebase Verification
The additional context provided by the scripts has revealed some important details:
- The
AclGuardandAclGuardDataare consistently used in other methods within theEventsControllerclass, suggesting a general pattern of using these guards for access control. - The specific method
campQuestionListand another methodgetEventsListByUserIdhave these guards commented out. This could indicate a special case or an exception to the general pattern. - The commit messages indicate recent work on role-based access control (ACL), specifically mentioning the addition of support for role-based ACL and modifications related to the
AuthGuard.
Given this information, it appears that the commenting out of the AclGuard and AclGuardData in the campQuestionList method might be intentional, possibly due to specific requirements or constraints that are not applicable to other methods. However, without explicit comments in the code or more detailed commit messages explaining why these lines were commented out, it's difficult to definitively confirm the intention.
To ensure clarity and maintainability, it would be beneficial for the development team to add comments in the code explaining why these guards are commented out in these specific methods, especially since it deviates from the common pattern observed in other parts of the controller.
Conclusion:
- The review comment is correct in pointing out the commented-out lines, and the need for clarification or consistency in access control is valid.
- It is recommended to add explanatory comments in the code or update the documentation to clarify the reasons for these exceptions to the general access control pattern.
Analysis chain
The commented-out AclGuard and AclGuardData in the campQuestionList method suggest a work-in-progress or reconsideration of access control for this method. Clarify the intention behind commenting these out and ensure consistent access control across methods.
The script output confirms that the AclGuard and AclGuardData annotations are commented out in the campQuestionList method as noted in the review comment. However, the intention behind this action is not clear from the provided script output alone. To fully understand why these lines were commented out, it would be beneficial to check for any comments in the surrounding code or relevant commit messages that might explain this change.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the ACL guards are intentionally commented out in the `campQuestionList` method.
cat src/src/events/events.controller.ts | grep -A 5 -B 5 "@Post('/camp-question-list')"
Length of output: 356
Script:
#!/bin/bash
# Check for comments around the commented out ACL guards in the `campQuestionList` method
rg -C 10 "@UseGuards\(AuthGuard\)" src/src/events/events.controller.ts
# Check for commit messages related to the changes in the `campQuestionList` method
git log -S "@UseGuards(AuthGuard)" -- src/src/events/events.controller.ts
Length of output: 5912
| const user_roles = req.mw_roles; | ||
| if(user_roles.includes('facilitator')){ | ||
| return res.statud(403).json({ | ||
| status:false, | ||
| message: 'FORBIDDEN', | ||
| data:{} | ||
| }) | ||
| }else if(user_roles.includes('staff')){ | ||
| let gqlquery = { | ||
| query : ` query MyQuery { | ||
| edit_requests_aggregate(where: { edit_req_approved_by: {_eq: ${req.mw_userid}}}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| }` | ||
| } | ||
| const result = await this.hasuraServiceFromServices.getData(gqlquery); | ||
| if(!result?.data && result.data.edit_requests_aggregate.aggregate.count > 0){ | ||
| return res.statud(403).json({ | ||
| status:false, | ||
| message: 'FORBIDDEN', | ||
| data:{} | ||
| }) |
There was a problem hiding this comment.
The getEditRequestListAdmin method correctly handles access control based on user roles. If the user has the 'facilitator' role, a forbidden status is returned, which aligns with the intended access control logic. However, there is a typo in the method name res.statud which should be corrected to res.status.
- res.statud(403).json({
+ res.status(403).json({Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const user_roles = req.mw_roles; | |
| if(user_roles.includes('facilitator')){ | |
| return res.statud(403).json({ | |
| status:false, | |
| message: 'FORBIDDEN', | |
| data:{} | |
| }) | |
| }else if(user_roles.includes('staff')){ | |
| let gqlquery = { | |
| query : ` query MyQuery { | |
| edit_requests_aggregate(where: { edit_req_approved_by: {_eq: ${req.mw_userid}}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| }` | |
| } | |
| const result = await this.hasuraServiceFromServices.getData(gqlquery); | |
| if(!result?.data && result.data.edit_requests_aggregate.aggregate.count > 0){ | |
| return res.statud(403).json({ | |
| status:false, | |
| message: 'FORBIDDEN', | |
| data:{} | |
| }) | |
| const user_roles = req.mw_roles; | |
| if(user_roles.includes('facilitator')){ | |
| return res.status(403).json({ | |
| status:false, | |
| message: 'FORBIDDEN', | |
| data:{} | |
| }) | |
| }else if(user_roles.includes('staff')){ | |
| let gqlquery = { | |
| query : ` query MyQuery { | |
| edit_requests_aggregate(where: { edit_req_approved_by: {_eq: ${req.mw_userid}}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| }` | |
| } | |
| const result = await this.hasuraServiceFromServices.getData(gqlquery); | |
| if(!result?.data && result.data.edit_requests_aggregate.aggregate.count > 0){ | |
| return res.status(403).json({ | |
| status:false, | |
| message: 'FORBIDDEN', | |
| data:{} | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
src/src/beneficiaries/beneficiaries.service.ts (1)
1060-1101: Refactor to use a service layer for business logic, reducing the complexity within the controller and adhering to the Single Responsibility Principle.
| @@ -16,6 +16,7 @@ import { UserHelperService } from '../helper/userHelper.service'; | |||
| import { HasuraService as HasuraServiceFromServices } from '../services/hasura/hasura.service'; | |||
| import { KeycloakService } from '../services/keycloak/keycloak.service'; | |||
| import { BeneficiariesCoreService } from './beneficiaries.core.service'; | |||
| import { AclHelper } from 'src/common/helpers/acl.helper'; | |||
There was a problem hiding this comment.
Consider removing unused imports to clean up the code.
| const ownershipWhereConditions = | ||
| await this.aclHelper.getOwnershipQueryConditionsForBeneficiaries( | ||
| req, | ||
| ); | ||
|
|
||
| let relationQuery; | ||
| if (ownershipWhereConditions.hasOwnProperty('program_faciltators')) { | ||
| relationQuery = `${ownershipWhereConditions['program_faciltators']}`; | ||
| } else if (ownershipWhereConditions.hasOwnProperty('program_users')) { | ||
| relationQuery = `${ownershipWhereConditions['program_users']}`; | ||
| } | ||
|
|
||
| let gqlQuery = { | ||
| query: `query MyQuery { | ||
| ownership : users_aggregate(where: {_and: [{program_beneficiaries: {facilitator_user: {program_faciltators: {${relationQuery}, academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}}}}}]}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| }; | ||
|
|
||
| console.log(gqlQuery.query); | ||
| const ownership_result = await this.hasuraServiceFromServices.getData( | ||
| gqlQuery, | ||
| ); | ||
| if ( | ||
| !( | ||
| ownership_result?.data && | ||
| ownership_result.data.ownership.aggregate.count > 0 | ||
| ) | ||
| ) { | ||
| return resp.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Refactor the method to separate concerns: one for ACL checks, another for data retrieval, and a third for CSV generation. This will improve maintainability and testability.
| const ownershipWhereConditions = | ||
| await this.aclHelper.getOwnershipQueryConditionsForBeneficiaries( | ||
| req, | ||
| ); | ||
| let relationQuery; | ||
| if (ownershipWhereConditions.hasOwnProperty('program_users')) { | ||
| relationQuery = `${ownershipWhereConditions['program_users']}`; | ||
| } else { | ||
| return resp.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } | ||
|
|
||
| let gqlQuery = { | ||
| query: `query MyQuery { | ||
| ownership : users_aggregate(where: {_and: [{program_beneficiaries: {facilitator_user: {program_faciltators: {${relationQuery}, academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}}}}}]}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| }; | ||
|
|
||
| console.log(gqlQuery.query); | ||
| const ownership_result = await this.hasuraServiceFromServices.getData( | ||
| gqlQuery, | ||
| ); | ||
| if ( | ||
| !( | ||
| ownership_result?.data && | ||
| ownership_result.data.ownership.aggregate.count > 0 | ||
| ) | ||
| ) { | ||
| return resp.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Consider implementing error handling for the asynchronous operations within this method to prevent unhandled promise rejections and improve reliability.
YoginiTayade
changed the title
|
| let gqlQuery = { | ||
| query: `query MyQuery { | ||
| ownership : program_beneficiaries_aggregate(where: {_and: [{academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}, facilitator_id: {_eq: ${req.mw_userid}}, status: {_in: ["identified", "ready_to_enroll", "enrolled", "not_enrolled", "enrollment_awaited", "enrollment_rejected", "enrolled_ip_verified", "registered_in_camp", "rejected", "dropout", "deactivated", "ineligible_for_pragati_camp", "_10th_passed"]}}, {user: {id: {_is_null: false}}}]}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| }; | ||
|
|
||
| console.log(gqlQuery.query); | ||
| const ownership_result = await this.hasuraServiceFromServices.getData( | ||
| gqlQuery, | ||
| ); | ||
| if ( | ||
| !( | ||
| ownership_result?.data && | ||
| ownership_result.data.ownership.aggregate.count > 0 | ||
| ) | ||
| ) { | ||
| return resp.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add error handling for the GraphQL query execution to ensure stability and provide meaningful feedback on failures.
| const ownershipWhereConditions = | ||
| await this.aclHelper.getOwnershipQueryConditionsForBeneficiaries( | ||
| req, | ||
| ); | ||
|
|
||
| let relationQuery; | ||
| if (ownershipWhereConditions.hasOwnProperty('program_faciltators')) { | ||
| relationQuery = `${ownershipWhereConditions['program_faciltators']}`; | ||
| } else if (ownershipWhereConditions.hasOwnProperty('program_users')) { | ||
| relationQuery = `${ownershipWhereConditions['program_users']}`; | ||
| } | ||
|
|
||
| let gqlQuery = { | ||
| query: `query MyQuery { | ||
| ownership : users_aggregate(where: {_and: [{program_beneficiaries: {facilitator_user: {program_faciltators: {${relationQuery}, program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}}}]}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| }; | ||
|
|
||
| console.log(gqlQuery.query); | ||
| const ownership_result = await this.hasuraServiceFromServices.getData( | ||
| gqlQuery, | ||
| ); | ||
| if ( | ||
| !( | ||
| ownership_result?.data && | ||
| ownership_result.data.ownership.aggregate.count > 0 | ||
| ) | ||
| ) { | ||
| return resp.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Improve error handling and separate the concerns of data fetching and response construction to enhance maintainability.
- if (ownership_result?.data && ownership_result.data.ownership.aggregate.count > 0) {
+ if (!ownership_result?.data || ownership_result.data.ownership.aggregate.count === 0) {
+ return resp.status(403).json({
+ success: false,
+ message: 'FORBIDDEN',
+ data: {},
+ });
+ }
+ // Proceed with data handlingCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const ownershipWhereConditions = | |
| await this.aclHelper.getOwnershipQueryConditionsForBeneficiaries( | |
| req, | |
| ); | |
| let relationQuery; | |
| if (ownershipWhereConditions.hasOwnProperty('program_faciltators')) { | |
| relationQuery = `${ownershipWhereConditions['program_faciltators']}`; | |
| } else if (ownershipWhereConditions.hasOwnProperty('program_users')) { | |
| relationQuery = `${ownershipWhereConditions['program_users']}`; | |
| } | |
| let gqlQuery = { | |
| query: `query MyQuery { | |
| ownership : users_aggregate(where: {_and: [{program_beneficiaries: {facilitator_user: {program_faciltators: {${relationQuery}, program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}}}]}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| console.log(gqlQuery.query); | |
| const ownership_result = await this.hasuraServiceFromServices.getData( | |
| gqlQuery, | |
| ); | |
| if ( | |
| !( | |
| ownership_result?.data && | |
| ownership_result.data.ownership.aggregate.count > 0 | |
| ) | |
| ) { | |
| return resp.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } | |
| const ownershipWhereConditions = | |
| await this.aclHelper.getOwnershipQueryConditionsForBeneficiaries( | |
| req, | |
| ); | |
| let relationQuery; | |
| if (ownershipWhereConditions.hasOwnProperty('program_faciltators')) { | |
| relationQuery = `${ownershipWhereConditions['program_faciltators']}`; | |
| } else if (ownershipWhereConditions.hasOwnProperty('program_users')) { | |
| relationQuery = `${ownershipWhereConditions['program_users']}`; | |
| } | |
| let gqlQuery = { | |
| query: `query MyQuery { | |
| ownership : users_aggregate(where: {_and: [{program_beneficiaries: {facilitator_user: {program_faciltators: {${relationQuery}, program_id: {_eq: ${req.mw_program_id}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}}}, academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}}}]}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| console.log(gqlQuery.query); | |
| const ownership_result = await this.hasuraServiceFromServices.getData( | |
| gqlQuery, | |
| ); | |
| if (!ownership_result?.data || ownership_result.data.ownership.aggregate.count === 0) { | |
| return resp.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } | |
| // Proceed with data handling |
| const ownershipWhereConditions = | ||
| await this.aclHelper.getOwnershipQueryConditionsForBeneficiaries( | ||
| req, | ||
| ); | ||
|
|
||
| let relationQuery; | ||
| if (ownershipWhereConditions.hasOwnProperty('program_faciltators')) { | ||
| relationQuery = `${ownershipWhereConditions['program_faciltators']}`; | ||
| } else if (ownershipWhereConditions.hasOwnProperty('program_users')) { | ||
| relationQuery = `${ownershipWhereConditions['program_users']}`; | ||
| } else { | ||
| return true; | ||
| } | ||
|
|
||
| let gqlQuery = { | ||
| query: `query MyQuery2 { | ||
| ownership : users_aggregate(where: {program_beneficiaries: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}, facilitator_user:{program_faciltators: {${relationQuery}}}}}) { | ||
| aggregate { | ||
| count | ||
| } | ||
| } | ||
| } | ||
|
|
||
| `, | ||
| }; | ||
|
|
||
| console.log(gqlQuery.query); | ||
| const ownership_result = await this.hasuraServiceFromServices.getData( | ||
| gqlQuery, | ||
| ); | ||
| if ( | ||
| !( | ||
| ownership_result?.data && | ||
| ownership_result.data.ownership.aggregate.count > 0 | ||
| ) | ||
| ) { | ||
| return resp.status(403).json({ | ||
| success: false, | ||
| message: 'FORBIDDEN', | ||
| data: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Enhance error handling to provide more detailed feedback and ensure robustness in operations.
- if (!ownership_result?.data || ownership_result.data.ownership.aggregate.count === 0) {
+ if (!ownership_result?.data || ownership_result.data.ownership.aggregate.count > 0) {
+ return resp.status(403).json({
+ success: false,
+ message: 'FORBIDDEN',
+ data: {},
+ });
+ }
+ // Continue with data processingCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const ownershipWhereConditions = | |
| await this.aclHelper.getOwnershipQueryConditionsForBeneficiaries( | |
| req, | |
| ); | |
| let relationQuery; | |
| if (ownershipWhereConditions.hasOwnProperty('program_faciltators')) { | |
| relationQuery = `${ownershipWhereConditions['program_faciltators']}`; | |
| } else if (ownershipWhereConditions.hasOwnProperty('program_users')) { | |
| relationQuery = `${ownershipWhereConditions['program_users']}`; | |
| } else { | |
| return true; | |
| } | |
| let gqlQuery = { | |
| query: `query MyQuery2 { | |
| ownership : users_aggregate(where: {program_beneficiaries: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}, facilitator_user:{program_faciltators: {${relationQuery}}}}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| console.log(gqlQuery.query); | |
| const ownership_result = await this.hasuraServiceFromServices.getData( | |
| gqlQuery, | |
| ); | |
| if ( | |
| !( | |
| ownership_result?.data && | |
| ownership_result.data.ownership.aggregate.count > 0 | |
| ) | |
| ) { | |
| return resp.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } | |
| const ownershipWhereConditions = | |
| await this.aclHelper.getOwnershipQueryConditionsForBeneficiaries( | |
| req, | |
| ); | |
| let relationQuery; | |
| if (ownershipWhereConditions.hasOwnProperty('program_faciltators')) { | |
| relationQuery = `${ownershipWhereConditions['program_faciltators']}`; | |
| } else if (ownershipWhereConditions.hasOwnProperty('program_users')) { | |
| relationQuery = `${ownershipWhereConditions['program_users']}`; | |
| } else { | |
| return true; | |
| } | |
| let gqlQuery = { | |
| query: `query MyQuery2 { | |
| ownership : users_aggregate(where: {program_beneficiaries: {academic_year_id: {_eq: ${req.mw_academic_year_id}}, program_id: {_eq: ${req.mw_program_id}}, facilitator_user:{program_faciltators: {${relationQuery}}}}}) { | |
| aggregate { | |
| count | |
| } | |
| } | |
| } | |
| `, | |
| }; | |
| console.log(gqlQuery.query); | |
| const ownership_result = await this.hasuraServiceFromServices.getData( | |
| gqlQuery, | |
| ); | |
| if (!ownership_result?.data || ownership_result.data.ownership.aggregate.count > 0) { | |
| return resp.status(403).json({ | |
| success: false, | |
| message: 'FORBIDDEN', | |
| data: {}, | |
| }); | |
| } | |
| // Continue with data processing |
https://tracker.tekdi.net/issues/215900
https://tracker.tekdi.net/issues/215928
I have ensured that following
Pull Request Checklistis taken care of before sending this PRSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores